Re: [PATCH V1 2/3] drivers: misc: dcc: Add driver support for Data Capture and Compare unit(DCC)

[Souradeep Chowdhury]

On 4/17/2023 1:11 PM, Greg Kroah-Hartman wrote:
> On Mon, Apr 17, 2023 at 12:26:23PM +0530, Souradeep Chowdhury wrote:
> > On 4/17/2023 11:47 AM, Greg Kroah-Hartman wrote:
> > > On Mon, Apr 17, 2023 at 11:31:46AM +0530, Souradeep Chowdhury wrote:
> > > > On 4/15/2023 11:09 AM, Greg Kroah-Hartman wrote:
> > > > > > +static void dcc_create_debug_dir(struct dcc_drvdata *drvdata)
> > > > > > +{
> > > > > > +	int i;
> > > > > > +	char list_num[10];
> > > > > > +	struct dentry *list;
> > > > > > +	struct device *dev = drvdata->dev;
> > > > > > +
> > > > > > +	drvdata->dbg_dir = debugfs_create_dir(dev_name(dev), NULL);
> > > > >
> > > > > You are creating a directory at the root of debugfs with just your
> > > > > device name?  While that will work, that feels very odd.  Please use a
> > > > > subdirectory.
> > > >
> > > > This is as per the comment on v17 of the patch series on DCC
> > > >
> > > > https://lore.kernel.org/linux-arm-kernel/6693993c-bd81-c974-a903-52a62bfec606@xxxxxxxx/
> > > >
> > > > "You never remove this dcc_dbg directory.  Why not?
> > > >
> > > > And since you don't, dcc_dbg could just be a local
> > > > variable here rather than being a global.  But it
> > > > seems to me this is the root directory you want to
> > > > remove when you're done."
> > >
> > > But that's not the issue.  The issue is that you are putting into
> > > /sys/kernel/debug/ a flat namespace of all of your devices.  Is that
> > > really what you want?  If so, why do you think this will never conflict
> > > with any other device in the system?
> >
> > Since we are going by the dev_name here which also contains the address
> > as per the example in the yaml binding, this will not conflict with other
> > dev_names in the system.
>
> That is not true at all.  dev_names are only unique per bus type.  There
> is nothing preventing any other bus from using the same name for their
> device as yours.
>
> So please, for the sake of your own sanity, just create a directory and
> dump all of your devices into it.  And for the sake of all of ours, as
> making the root of debugfs full of random device names is a mess, don't
> you think?  What would happen if all drivers did that?

Ack

> > > > As per upstream discussions it was decided that debugfs will be a suitable
> > > > interface for DCC. More on this in the following link:-
> > > >
> > > > https://lore.kernel.org/linux-arm-kernel/20221228172825.r32vpphbdulaldvv@xxxxxxxxxxx/
> > >
> > > debugfs is not a suitable interface for anything that is "real" as
> > > that's not what it is there for.  Most systems disable debugfs entirely
> > > (i.e. Android), or prevent any normal user from accessing it, so this
> > > api you are creating will not be able to be used by anyone.
> > >
> > > debugfs is for debugging, not for anything that the system functionality
> > > relies on to work properly.
> >
> > DCC is a debugging hardware which stores the values of the configured
> > register address on a system crash on it's dedicated sram. These register
> > addresses are configured by the user through the debugfs interface. Also on
> > the platforms where debugfs is disabled there is an alternative of using
> > bootconfig suggested to configure the register values during boot-time.
> > There is an ongoing patch series for that as follows:-
> >
> > https://lore.kernel.org/linux-arm-kernel/cover.1675054375.git.quic_schowdhu@xxxxxxxxxxx/T/
>
> But again, debugfs is not even mounted in most systems, so how are they
> going to interact with your hardware?  Restricting it to debugfs feels
> very odd to me, but hey, it's your code, I guess you don't want anyone
> to use it :)
>
> good luck!
>
> greg k-h