| Message ID | 20170404032514.31832-1-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
Hi, On 04-04-17 05:25, Dmitry Torokhov wrote: > I2C bus has both i2c clients and adapter devices, so we must be careful in > notifier code and verify that we are actually dealing with an i2c client > before using it as such. > > Fixes: cef9dd85acd7 ("platform/x86: add support for devices with Silead...") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thank you for fixing this, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/platform/x86/silead_dmi.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/silead_dmi.c > index 02e11fdbf375..7f1049951d1c 100644 > --- a/drivers/platform/x86/silead_dmi.c > +++ b/drivers/platform/x86/silead_dmi.c > @@ -75,9 +75,8 @@ static const struct dmi_system_id silead_ts_dmi_table[] = { > { }, > }; > > -static void silead_ts_dmi_add_props(struct device *dev) > +static void silead_ts_dmi_add_props(struct i2c_client *client) > { > - struct i2c_client *client = to_i2c_client(dev); > const struct dmi_system_id *dmi_id; > const struct silead_ts_dmi_data *ts_data; > int error; > @@ -87,11 +86,13 @@ static void silead_ts_dmi_add_props(struct device *dev) > return; > > ts_data = dmi_id->driver_data; > - if (has_acpi_companion(dev) && > + if (has_acpi_companion(&client->dev) && > !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) { > - error = device_add_properties(dev, ts_data->properties); > + error = device_add_properties(&client->dev, > + ts_data->properties); > if (error) > - dev_err(dev, "failed to add properties: %d\n", error); > + dev_err(&client->dev, > + "failed to add properties: %d\n", error); > } > } > > @@ -99,10 +100,13 @@ static int silead_ts_dmi_notifier_call(struct notifier_block *nb, > unsigned long action, void *data) > { > struct device *dev = data; > + struct i2c_client *client; > > switch (action) { > case BUS_NOTIFY_ADD_DEVICE: > - silead_ts_dmi_add_props(dev); > + client = i2c_verify_client(dev); > + if (client) > + silead_ts_dmi_add_props(client); > break; > > default: >
On Tue, Apr 4, 2017 at 6:25 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > I2C bus has both i2c clients and adapter devices, so we must be careful in > notifier code and verify that we are actually dealing with an i2c client > before using it as such. > -static void silead_ts_dmi_add_props(struct device *dev) > +static void silead_ts_dmi_add_props(struct i2c_client *client) > { > - struct i2c_client *client = to_i2c_client(dev); I would replace this by struct device *dev = &client->dev; Otherwise looks good for me. > const struct dmi_system_id *dmi_id; > const struct silead_ts_dmi_data *ts_data; > int error; > @@ -87,11 +86,13 @@ static void silead_ts_dmi_add_props(struct device *dev) > return; > > ts_data = dmi_id->driver_data; > - if (has_acpi_companion(dev) && > + if (has_acpi_companion(&client->dev) && > !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) { > - error = device_add_properties(dev, ts_data->properties); > + error = device_add_properties(&client->dev, > + ts_data->properties); > if (error) > - dev_err(dev, "failed to add properties: %d\n", error); > + dev_err(&client->dev, > + "failed to add properties: %d\n", error); > }
On Tue, Apr 04, 2017 at 04:45:05PM +0300, Andy Shevchenko wrote: > On Tue, Apr 4, 2017 at 6:25 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > I2C bus has both i2c clients and adapter devices, so we must be careful in > > notifier code and verify that we are actually dealing with an i2c client > > before using it as such. > > > -static void silead_ts_dmi_add_props(struct device *dev) > > +static void silead_ts_dmi_add_props(struct i2c_client *client) > > { > > > - struct i2c_client *client = to_i2c_client(dev); > > I would replace this by > struct device *dev = &client->dev; > > Otherwise looks good for me. Andy, this series looks like a candidate for 4.11-fixes. We're already at rc5 though, so if we are going to do that, I'd like to see a stronger statement in the commit log about how this issue manifests currently - if it does.
On Tue, Apr 4, 2017 at 7:05 PM, Darren Hart <dvhart@infradead.org> wrote: > On Tue, Apr 04, 2017 at 04:45:05PM +0300, Andy Shevchenko wrote: >> On Tue, Apr 4, 2017 at 6:25 AM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > I2C bus has both i2c clients and adapter devices, so we must be careful in >> > notifier code and verify that we are actually dealing with an i2c client >> > before using it as such. >> >> > -static void silead_ts_dmi_add_props(struct device *dev) >> > +static void silead_ts_dmi_add_props(struct i2c_client *client) >> > { >> >> > - struct i2c_client *client = to_i2c_client(dev); >> >> I would replace this by >> struct device *dev = &client->dev; >> >> Otherwise looks good for me. > > Andy, this series looks like a candidate for 4.11-fixes. We're already at rc5 > though, so if we are going to do that, I'd like to see a stronger statement in > the commit log about how this issue manifests currently - if it does. It makes less changes for any (potentially) backported code. I'm not insisting and even can do myself.
On Tue, Apr 04, 2017 at 07:08:28PM +0300, Andy Shevchenko wrote: > On Tue, Apr 4, 2017 at 7:05 PM, Darren Hart <dvhart@infradead.org> wrote: > > On Tue, Apr 04, 2017 at 04:45:05PM +0300, Andy Shevchenko wrote: > >> On Tue, Apr 4, 2017 at 6:25 AM, Dmitry Torokhov > >> <dmitry.torokhov@gmail.com> wrote: > >> > I2C bus has both i2c clients and adapter devices, so we must be careful in > >> > notifier code and verify that we are actually dealing with an i2c client > >> > before using it as such. > >> > >> > -static void silead_ts_dmi_add_props(struct device *dev) > >> > +static void silead_ts_dmi_add_props(struct i2c_client *client) > >> > { > >> > >> > - struct i2c_client *client = to_i2c_client(dev); > >> > >> I would replace this by > >> struct device *dev = &client->dev; > >> > >> Otherwise looks good for me. > > > > Andy, this series looks like a candidate for 4.11-fixes. We're already at rc5 > > though, so if we are going to do that, I'd like to see a stronger statement in > > the commit log about how this issue manifests currently - if it does. > > It makes less changes for any (potentially) backported code. > I'm not insisting and even can do myself. Sorry, I was referring to the series itself, not your feedback above. You assigned this to yourself in patchwork, so I was just noting that this patch series may be a candidate for fixes to 4.11, rather than testing/for-next for 4.12. Your call.
On Tue, Apr 4, 2017 at 7:31 PM, Darren Hart <dvhart@infradead.org> wrote: > On Tue, Apr 04, 2017 at 07:08:28PM +0300, Andy Shevchenko wrote: >> On Tue, Apr 4, 2017 at 7:05 PM, Darren Hart <dvhart@infradead.org> wrote: >> > On Tue, Apr 04, 2017 at 04:45:05PM +0300, Andy Shevchenko wrote: >> >> On Tue, Apr 4, 2017 at 6:25 AM, Dmitry Torokhov >> >> <dmitry.torokhov@gmail.com> wrote: >> >> > I2C bus has both i2c clients and adapter devices, so we must be careful in >> >> > notifier code and verify that we are actually dealing with an i2c client >> >> > before using it as such. >> >> >> >> > -static void silead_ts_dmi_add_props(struct device *dev) >> >> > +static void silead_ts_dmi_add_props(struct i2c_client *client) >> >> > { >> >> >> >> > - struct i2c_client *client = to_i2c_client(dev); >> >> >> >> I would replace this by >> >> struct device *dev = &client->dev; >> >> >> >> Otherwise looks good for me. >> > >> > Andy, this series looks like a candidate for 4.11-fixes. We're already at rc5 >> > though, so if we are going to do that, I'd like to see a stronger statement in >> > the commit log about how this issue manifests currently - if it does. >> >> It makes less changes for any (potentially) backported code. >> I'm not insisting and even can do myself. > > Sorry, I was referring to the series itself, not your feedback above. You > assigned this to yourself in patchwork, so I was just noting that this patch > series may be a candidate for fixes to 4.11, rather than testing/for-next for > 4.12. Your call. Ah, thanks Darren for clarification.
On Tue, Apr 04, 2017 at 07:33:30PM +0300, Andy Shevchenko wrote: > On Tue, Apr 4, 2017 at 7:31 PM, Darren Hart <dvhart@infradead.org> wrote: > > On Tue, Apr 04, 2017 at 07:08:28PM +0300, Andy Shevchenko wrote: > >> On Tue, Apr 4, 2017 at 7:05 PM, Darren Hart <dvhart@infradead.org> wrote: > >> > On Tue, Apr 04, 2017 at 04:45:05PM +0300, Andy Shevchenko wrote: > >> >> On Tue, Apr 4, 2017 at 6:25 AM, Dmitry Torokhov > >> >> <dmitry.torokhov@gmail.com> wrote: > >> >> > I2C bus has both i2c clients and adapter devices, so we must be careful in > >> >> > notifier code and verify that we are actually dealing with an i2c client > >> >> > before using it as such. > >> >> > >> >> > -static void silead_ts_dmi_add_props(struct device *dev) > >> >> > +static void silead_ts_dmi_add_props(struct i2c_client *client) > >> >> > { > >> >> > >> >> > - struct i2c_client *client = to_i2c_client(dev); > >> >> > >> >> I would replace this by > >> >> struct device *dev = &client->dev; > >> >> > >> >> Otherwise looks good for me. > >> > > >> > Andy, this series looks like a candidate for 4.11-fixes. We're already at rc5 > >> > though, so if we are going to do that, I'd like to see a stronger statement in > >> > the commit log about how this issue manifests currently - if it does. > >> > >> It makes less changes for any (potentially) backported code. > >> I'm not insisting and even can do myself. OK, I'll leave it to you if you decide you'd rather have a temporary for 'dev'. > > > > Sorry, I was referring to the series itself, not your feedback above. You > > assigned this to yourself in patchwork, so I was just noting that this patch > > series may be a candidate for fixes to 4.11, rather than testing/for-next for > > 4.12. Your call. > > Ah, thanks Darren for clarification. I think this can easily wait for 4.12 if you are concerned about regressions. i2c_adapter is large enough so we won't reference unallocated memory and we only read from client->name so we should not perturb anything if we actually are dealing with i2c_adapter, and the data we'll read is unlikely to match to ACPI name we interested in. The 2nd patch is mere optimization. Thanks.
diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/silead_dmi.c index 02e11fdbf375..7f1049951d1c 100644 --- a/drivers/platform/x86/silead_dmi.c +++ b/drivers/platform/x86/silead_dmi.c @@ -75,9 +75,8 @@ static const struct dmi_system_id silead_ts_dmi_table[] = { { }, }; -static void silead_ts_dmi_add_props(struct device *dev) +static void silead_ts_dmi_add_props(struct i2c_client *client) { - struct i2c_client *client = to_i2c_client(dev); const struct dmi_system_id *dmi_id; const struct silead_ts_dmi_data *ts_data; int error; @@ -87,11 +86,13 @@ static void silead_ts_dmi_add_props(struct device *dev) return; ts_data = dmi_id->driver_data; - if (has_acpi_companion(dev) && + if (has_acpi_companion(&client->dev) && !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) { - error = device_add_properties(dev, ts_data->properties); + error = device_add_properties(&client->dev, + ts_data->properties); if (error) - dev_err(dev, "failed to add properties: %d\n", error); + dev_err(&client->dev, + "failed to add properties: %d\n", error); } } @@ -99,10 +100,13 @@ static int silead_ts_dmi_notifier_call(struct notifier_block *nb, unsigned long action, void *data) { struct device *dev = data; + struct i2c_client *client; switch (action) { case BUS_NOTIFY_ADD_DEVICE: - silead_ts_dmi_add_props(dev); + client = i2c_verify_client(dev); + if (client) + silead_ts_dmi_add_props(client); break; default:
I2C bus has both i2c clients and adapter devices, so we must be careful in notifier code and verify that we are actually dealing with an i2c client before using it as such. Fixes: cef9dd85acd7 ("platform/x86: add support for devices with Silead...") Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/platform/x86/silead_dmi.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)