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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Anyway, I see that pattern "to_<something>_driver" is commonly used.




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux