Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 5 Oct 2021 12:27:09 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc:     Lee Jones <lee.jones@linaro.org>, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: most: dim2: fix device registration
Message-ID: <YVwofSvwGTv3kHjh@kroah.com>
References: <20210929205619.2800-1-nikita.yoush@cogentembedded.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210929205619.2800-1-nikita.yoush@cogentembedded.com>
Precedence: bulk

On Wed, Sep 29, 2021 at 11:56:20PM +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?

> 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.  Whenever you have to list a bunch of
different things you are doing in a single change, that's a hint that
this should be more than one patch.

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?

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.

thanks,

greg k-h