Re: [PATCH] staging: most: dim2: fix device registration

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Oct 05, 2021 at 04:33:02PM +0300, Nikita Yushchenko wrote:
> > > Commit 723de0f9171e ("staging: most: remove device from interface
> > > structure") moved registration of driver-provided struct device to
> > > the most subsystem, but did not properly update dim2 driver to
> > > work with that change.
> > > 
> > > After most subsystem passes driver's dev to register_device(), it
> > > becomes refcounted, and can be only deallocated in the release method.
> > > Provide that by:
> > > - not using devres to allocate the device,
> > > - moving shutdown actions from _remove() to the device release method,
> > > - not calling shutdown actions in _probe() after the device becomes
> > >    refcounted.
> > 
> > Should this be 3 patches?
> 
> But these three items are deeply interconnected, and fix the issue together.
> Must not manually free device structure passed to register_device(), thus
> must not allocate via devres (because otherwise, devres will free it). Once
> not using devres for it, must deallocate it somehow else, thus must rework
> the release paths.

Ok, but that was obvious.

> > > Also, driver used to register it's dev itself, to provide a custom
> > > attribute. With the modified most subsystem, this causes duplicate
> > > registration of the same device object. Fix that by adding that custom
> > > attribute to the platform device - that is a better location for
> > > a platform-specific attribute anyway.
> > 
> > Nope, it should be 4 patches.
> 
> Unlike the above three, this item could be separated.
> Will split into two patches now - the first for this (and with fix to the
> attributes issue noted below) and the second for proper device releasing.
> 
> > Also, why have you not cc:ed the original author of the commit you are
> > "fixing" here?   They are the maintainer of this code, right?
> 
> I was under impression that "git send-email" does that automatically...

Nope.

> CCing them now.

They don't see the change :(

> > One note on your change that would keep me from accepting it even if all
> > of the above was not an issue:
> > 
> > > diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
> > > index c85b2cdcdca3..22836c8ed554 100644
> > > --- a/drivers/staging/most/dim2/sysfs.c
> > > +++ b/drivers/staging/most/dim2/sysfs.c
> > > @@ -39,11 +39,10 @@ static const struct attribute_group *dev_attr_groups[] = {
> > >   int dim2_sysfs_probe(struct device *dev)
> > >   {
> > > -	dev->groups = dev_attr_groups;
> > > -	return device_register(dev);
> > > +	return sysfs_create_groups(&dev->kobj, dev_attr_groups);
> > 
> > No driver code should ever be calling a sysfs_* function, which is a
> > huge hint that this is incorrect.
> > 
> > You also just raced with userspace and lost, please use the default
> > attributes for the driver or bus for this, but NEVER manually add and
> > remove sysfs files, that way lies madness and hard to maintain code.
> I'm aware of this race, but still creating attributes on device probe is under wide use in the kernel:
> 
> nikita@cobook:~/kernel$ grep -r device_create_file drivers | wc -l
> 448

Just because there are bad examples, does not mean you should add more.

And not all of these are wrong, but do you know when they are not wrong?

> Still, in case of dim2 driver, moving to driver's dev_groups is
> trivial. Preparing that patch now.

That should have already been done, right?

thanks,

greg k-h