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

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

[Dmitry Torokhov]

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

[PATCH 2/2] platform/x86: silead_dmi - abort early if DMI does not match

[Dmitry Torokhov]

There is no point in registering I2C bus notifier if DMI data does not
match.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/platform/x86/silead_dmi.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/silead_dmi.c
index 7f1049951d1c..0eeb49e51c43 100644
--- a/drivers/platform/x86/silead_dmi.c
+++ b/drivers/platform/x86/silead_dmi.c
@@ -75,21 +75,16 @@ static const struct dmi_system_id silead_ts_dmi_table[] = {
 	{ },
 };
 
+static const struct silead_ts_dmi_data *silead_ts_data;
+
 static void silead_ts_dmi_add_props(struct i2c_client *client)
 {
-	const struct dmi_system_id *dmi_id;
-	const struct silead_ts_dmi_data *ts_data;
 	int error;
 
-	dmi_id = dmi_first_match(silead_ts_dmi_table);
-	if (!dmi_id)
-		return;
-
-	ts_data = dmi_id->driver_data;
 	if (has_acpi_companion(&client->dev) &&
-	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
+	    !strncmp(silead_ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
 		error = device_add_properties(&client->dev,
-					      ts_data->properties);
+					      silead_ts_data->properties);
 		if (error)
 			dev_err(&client->dev,
 				"failed to add properties: %d\n", error);
@@ -122,8 +117,15 @@ static struct notifier_block silead_ts_dmi_notifier = {
 
 static int __init silead_ts_dmi_init(void)
 {
+	const struct dmi_system_id *dmi_id;
 	int error;
 
+	dmi_id = dmi_first_match(silead_ts_dmi_table);
+	if (!dmi_id)
+		return 0; /* Not an error */
+
+	silead_ts_data = dmi_id->driver_data;
+
 	error = bus_register_notifier(&i2c_bus_type, &silead_ts_dmi_notifier);
 	if (error)
 		pr_err("%s: failed to register i2c bus notifier: %d\n",
-- 
2.12.2.715.g7642488e1d-goog

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

[Hans de Goede]

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

Re: [PATCH 2/2] platform/x86: silead_dmi - abort early if DMI does not match

[Hans de Goede]

Hi,

On 04-04-17 05:25, Dmitry Torokhov wrote:
> There is no point in registering I2C bus notifier if DMI data does not
> match.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Good idea, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/platform/x86/silead_dmi.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/silead_dmi.c
> index 7f1049951d1c..0eeb49e51c43 100644
> --- a/drivers/platform/x86/silead_dmi.c
> +++ b/drivers/platform/x86/silead_dmi.c
> @@ -75,21 +75,16 @@ static const struct dmi_system_id silead_ts_dmi_table[] = {
>  	{ },
>  };
>
> +static const struct silead_ts_dmi_data *silead_ts_data;
> +
>  static void silead_ts_dmi_add_props(struct i2c_client *client)
>  {
> -	const struct dmi_system_id *dmi_id;
> -	const struct silead_ts_dmi_data *ts_data;
>  	int error;
>
> -	dmi_id = dmi_first_match(silead_ts_dmi_table);
> -	if (!dmi_id)
> -		return;
> -
> -	ts_data = dmi_id->driver_data;
>  	if (has_acpi_companion(&client->dev) &&
> -	    !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
> +	    !strncmp(silead_ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
>  		error = device_add_properties(&client->dev,
> -					      ts_data->properties);
> +					      silead_ts_data->properties);
>  		if (error)
>  			dev_err(&client->dev,
>  				"failed to add properties: %d\n", error);
> @@ -122,8 +117,15 @@ static struct notifier_block silead_ts_dmi_notifier = {
>
>  static int __init silead_ts_dmi_init(void)
>  {
> +	const struct dmi_system_id *dmi_id;
>  	int error;
>
> +	dmi_id = dmi_first_match(silead_ts_dmi_table);
> +	if (!dmi_id)
> +		return 0; /* Not an error */
> +
> +	silead_ts_data = dmi_id->driver_data;
> +
>  	error = bus_register_notifier(&i2c_bus_type, &silead_ts_dmi_notifier);
>  	if (error)
>  		pr_err("%s: failed to register i2c bus notifier: %d\n",
>

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

[Andy Shevchenko]

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

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

[Darren Hart]

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

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

[Andy Shevchenko]

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

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

[Darren Hart]

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

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

[Andy Shevchenko]

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

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

[Dmitry Torokhov]

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

Re: [PATCH 2/2] platform/x86: silead_dmi - abort early if DMI does not match

[Andy Shevchenko]

On Tue, Apr 4, 2017 at 6:25 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> There is no point in registering I2C bus notifier if DMI data does not
> match.
>

Both applied to testing.
Please check I didn't mess things up.

Thanks!

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/platform/x86/silead_dmi.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/silead_dmi.c b/drivers/platform/x86/silead_dmi.c
> index 7f1049951d1c..0eeb49e51c43 100644
> --- a/drivers/platform/x86/silead_dmi.c
> +++ b/drivers/platform/x86/silead_dmi.c
> @@ -75,21 +75,16 @@ static const struct dmi_system_id silead_ts_dmi_table[] = {
>         { },
>  };
>
> +static const struct silead_ts_dmi_data *silead_ts_data;
> +
>  static void silead_ts_dmi_add_props(struct i2c_client *client)
>  {
> -       const struct dmi_system_id *dmi_id;
> -       const struct silead_ts_dmi_data *ts_data;
>         int error;
>
> -       dmi_id = dmi_first_match(silead_ts_dmi_table);
> -       if (!dmi_id)
> -               return;
> -
> -       ts_data = dmi_id->driver_data;
>         if (has_acpi_companion(&client->dev) &&
> -           !strncmp(ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
> +           !strncmp(silead_ts_data->acpi_name, client->name, I2C_NAME_SIZE)) {
>                 error = device_add_properties(&client->dev,
> -                                             ts_data->properties);
> +                                             silead_ts_data->properties);
>                 if (error)
>                         dev_err(&client->dev,
>                                 "failed to add properties: %d\n", error);
> @@ -122,8 +117,15 @@ static struct notifier_block silead_ts_dmi_notifier = {
>
>  static int __init silead_ts_dmi_init(void)
>  {
> +       const struct dmi_system_id *dmi_id;
>         int error;
>
> +       dmi_id = dmi_first_match(silead_ts_dmi_table);
> +       if (!dmi_id)
> +               return 0; /* Not an error */
> +
> +       silead_ts_data = dmi_id->driver_data;
> +
>         error = bus_register_notifier(&i2c_bus_type, &silead_ts_dmi_notifier);
>         if (error)
>                 pr_err("%s: failed to register i2c bus notifier: %d\n",
> --
> 2.12.2.715.g7642488e1d-goog
>



-- 
With Best Regards,
Andy Shevchenko