Re: [PATCH v3 2/6] i2c: i801: Use a different adapter-name for IDF adapters

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/22/24 5:07 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:29:21 Hans de Goede wrote:
>> Hi,
>>
>> On 6/22/24 4:23 PM, Pali Rohár wrote:
>>> On Saturday 22 June 2024 16:14:11 Hans de Goede wrote:
>>>> Hi Pali,
>>>>
>>>> On 6/22/24 4:08 PM, Pali Rohár wrote:
>>>>> On Saturday 22 June 2024 15:56:03 Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 6/22/24 2:46 PM, Pali Rohár wrote:
>>>>>>> On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
>>>>>>>> On chipsets with a second 'Integrated Device Function' SMBus controller use
>>>>>>>> a different adapter-name for the second IDF adapter.
>>>>>>>>
>>>>>>>> This allows platform glue code which is looking for the primary i801
>>>>>>>> adapter to manually instantiate i2c_clients on to differentiate
>>>>>>>> between the 2.
>>>>>>>>
>>>>>>>> This allows such code to find the primary i801 adapter by name, without
>>>>>>>> needing to duplicate the PCI-ids to feature-flags mapping from i2c-i801.c.
>>>>>>>>
>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>> ---
>>>>>>>>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
>>>>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>>>>>> index d2d2a6dbe29f..5ac5bbd60d45 100644
>>>>>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>>>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>>>>>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>>>>>  
>>>>>>>>  	i801_add_tco(priv);
>>>>>>>>  
>>>>>>>> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>>>> -		"SMBus I801 adapter at %04lx", priv->smba);
>>>>>>>> +	if (priv->features & FEATURE_IDF)
>>>>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>>>> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
>>>>>>>> +	else
>>>>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>>>> +			"SMBus I801 adapter at %04lx", priv->smba);
>>>>>>>> +
>>>>>>>
>>>>>>> User visible name is identifier for user / human.
>>>>>>>
>>>>>>> If somebody is going to read this code in next 10 years then can ask
>>>>>>> question why to have different name for IDF FEATURE and not also for
>>>>>>> other features? And can come to conclusion to unify all names to be
>>>>>>> same (why not? it is user identifier).
>>>>>>
>>>>>> That is a good point, I'll add a comment about this for the next
>>>>>> version.
>>>>>>
>>>>>>> Depending on user names between different kernel subsystem is fragile,
>>>>>>> specially for future as rename can happen.
>>>>>>
>>>>>> Relying no devices names to find devices is standard practice. E.g.
>>>>>> this is how 99% of the platform drivers bind to platform devices
>>>>>> by the driver and the device having the same name.
>>>>>
>>>>> But here it is adapter name which is more likely description, not the
>>>>> device name which is used for binding.
>>>>
>>>> It is still matching on a name.
>>>>
>>>>>>> If you are depending on FEATURE_IDF flag then check for the flag
>>>>>>> directly, and not hiding the flag by serializing it into the user
>>>>>>> visible name (char[] variable) and then de-serializing it in different
>>>>>>> kernel subsystem. If the flag is not exported yet then export it via
>>>>>>> some function or other API.
>>>>>>
>>>>>> Exporting this through some new function is non trivial and adds
>>>>>> extra dependencies between modules, causing issues when one is builtin
>>>>>> and the other is build as a module.
>>>>>
>>>>> Access to "struct i801_priv *" is not possible? For example via
>>>>> dev_get_drvdata() on "struct device *" which you have in
>>>>> smo8800_find_i801()?
>>>>>
>>>>> Because if it is possible then you can create an inline function in some
>>>>> shared header file which access this flag. Not perfect (as accessing
>>>>> private data is not the best thing) but can avoid dependences between
>>>>> modules.
>>>>
>>>> Prodding inside another drivers private driver struct is a big nono
>>>> and much much more fragile then the name checking.
>>>
>>> I know, that is why I wrote to access this structure and flags in
>>> separate function which can be an inline in e.g. i2c-i801.h header file.
>>
>> We would still need to be very very sure the device we are calling that
>> function on actually has the i2c-i801.c driver bound to it, so that
>> e.g. we are not dereferencing a NULL pointer drvdata, or worse
>> poking at some other drivers private data because we are calling
>> the helper on the wrong device.
>>
>> To make sure that is the case we would need to e.g. check that:
>> a) The device in question is an i2c adapter
>> b) the adapter name matches, so we would still be doing name matching ...
>>
>> Really just matching on the adapter name is by far the cleanest
>> option here.
>>
>> Regards,
>>
>> Hans
> 
> Ok, I have looked at it now.
> 
> For a) you are already using i2c_verify_adapter(). So this part is done.
> 
> For b) you do not need to check adapter name, but rather adapter driver
> name (in case driver data are begin accessed). And I think you can use
> dev->driver.name to get it.

This would still be checking a name and on top of that requires adding
a new helper to i2c-i801.c so we are still checking a name and the code
has gotten more complex.

Regards,

Hans