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;
Subject: Re: [PATCH] staging: most: dim2: fix device registration
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Lee Jones <lee.jones@linaro.org>, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org,
        Christian Gromm <christian.gromm@microchip.com>
References: <20210929205619.2800-1-nikita.yoush@cogentembedded.com>
 <YVwofSvwGTv3kHjh@kroah.com>
From:   Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Message-ID: <d309b4cf-12f5-5f49-fcbe-3141dff9e73f@cogentembedded.com>
Date:   Tue, 5 Oct 2021 16:33:02 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <YVwofSvwGTv3kHjh@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

Perhaps I just shall not go into these details in the commit message.

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

CCing them now.

> 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

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

Nikita