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?
> > > 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
