Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1615686 > unrolled thread

[PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-04-04 05:30 +0200
    Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices  as i2c_clients Hans de Goede <hdegoede@redhat.com> - 2017-04-04 12:00 +0200
    Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices  as i2c_clients Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 15:50 +0200
      Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices  as i2c_clients Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 18:10 +0200
        Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices  as i2c_clients Darren Hart <dvhart@infradead.org> - 2017-04-04 18:40 +0200
          Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices  as i2c_clients Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-04 18:40 +0200
            Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices  as i2c_clients Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-04-04 19:20 +0200
      Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices  as i2c_clients Darren Hart <dvhart@infradead.org> - 2017-04-04 18:10 +0200

#1615686 — [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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(-)

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:
-- 
2.12.2.715.g7642488e1d-goog

[toc] | [next] | [standalone]

#1615844 — Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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

[toc] | [prev] | [next] | [standalone]

#1616045 — Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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);
>         }



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1616173 — Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1616198 — Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1616200 — Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1616219 — Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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.

-- 
Dmitry

[toc] | [prev] | [next] | [standalone]

#1616178 — Re: [PATCH 1/2] platform/x86: silead_dmi - do not treat all devices as i2c_clients

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.

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web