| Message ID | 20240621122503.10034-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand |
On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> 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. ... > - 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); Alternatively this can be snprintf(priv->adapter.name, sizeof(priv->adapter.name), "SMBus %s adapter at %04lx", priv->features & FEATURE_IDF ? "I801 IDF" : "I801", priv->smba); This will save a few bytes here and there.
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). Depending on user names between different kernel subsystem is fragile, specially for future as rename can happen. 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. Using user name via char[] for internal flags is misusing user visible name structures. > err = i2c_add_adapter(&priv->adapter); > if (err) { > platform_device_unregister(priv->tco_pdev); > -- > 2.45.1 >
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. > 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. The name check OTOH allows the modules to be less tightly coupled and there is plenty of precedence for using a name check here. Regards, Hans >> err = i2c_add_adapter(&priv->adapter); >> if (err) { >> platform_device_unregister(priv->tco_pdev); >> -- >> 2.45.1 >> >
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. > > 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. > The name check OTOH allows the modules to be less tightly coupled > and there is plenty of precedence for using a name check here. > > Regards, > > Hans > > > > >> err = i2c_add_adapter(&priv->adapter); > >> if (err) { > >> platform_device_unregister(priv->tco_pdev); > >> -- > >> 2.45.1 > >> > > >
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. Regards, Hans
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. > Regards, > > Hans > >
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
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. Anyway, I see that pattern "to_<something>_driver" is commonly used.
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
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); + err = i2c_add_adapter(&priv->adapter); if (err) { platform_device_unregister(priv->tco_pdev);
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(-)