| Message ID | 20190212120523.27463-1-hong.liu@intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 0d28f49412405d87d3aae83da255070a46e67627 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [1/4] HID: intel-ish-hid: avoid binding wrong ishtp_cl_device | expand |
On Tue, 12 Feb 2019, Hong Liu wrote: > When performing a warm reset in ishtp bus driver, the ishtp_cl_device > will not be removed, its fw_client still points to the already freed > ishtp_device.fw_clients array. > > Later after driver finishing ishtp client enumeration, this dangling > pointer may cause driver to bind the wrong ishtp_cl_device to the new > client, causing wrong callback to be called for messages intended for > the new client. > > This helps in development of firmware where frequent switching of > firmwares is required without Linux reboot. > > Signed-off-by: Hong Liu <hong.liu@intel.com> > Tested-by: Hongyan Song <hongyan.song@intel.com> Srinivas, could you please ack this series? Thanks. > --- > drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c > index 728dc6d4561a..a271d6d169b1 100644 > --- a/drivers/hid/intel-ish-hid/ishtp/bus.c > +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c > @@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl) > spin_lock_irqsave(&cl->dev->device_list_lock, flags); > list_for_each_entry(cl_device, &cl->dev->device_list, > device_link) { > - if (cl_device->fw_client->client_id == cl->fw_client_id) { > + if (cl_device->fw_client && > + cl_device->fw_client->client_id == cl->fw_client_id) { > cl->device = cl_device; > rv = 0; > break; > @@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev, > spin_lock_irqsave(&ishtp_dev->device_list_lock, flags); > list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list, > device_link) { > + cl_device->fw_client = NULL; > if (warm_reset && cl_device->reference_count) > continue; > > -- > 2.20.1 >
On Tue, 2019-02-12 at 20:05 +0800, Hong Liu wrote: > When performing a warm reset in ishtp bus driver, the ishtp_cl_device > will not be removed, its fw_client still points to the already freed > ishtp_device.fw_clients array. > > Later after driver finishing ishtp client enumeration, this dangling > pointer may cause driver to bind the wrong ishtp_cl_device to the new > client, causing wrong callback to be called for messages intended for > the new client. > > This helps in development of firmware where frequent switching of > firmwares is required without Linux reboot. > > Signed-off-by: Hong Liu <hong.liu@intel.com> > Tested-by: Hongyan Song <hongyan.song@intel.com> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c > b/drivers/hid/intel-ish-hid/ishtp/bus.c > index 728dc6d4561a..a271d6d169b1 100644 > --- a/drivers/hid/intel-ish-hid/ishtp/bus.c > +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c > @@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl) > spin_lock_irqsave(&cl->dev->device_list_lock, flags); > list_for_each_entry(cl_device, &cl->dev->device_list, > device_link) { > - if (cl_device->fw_client->client_id == cl- > >fw_client_id) { > + if (cl_device->fw_client && > + cl_device->fw_client->client_id == cl- > >fw_client_id) { > cl->device = cl_device; > rv = 0; > break; > @@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct > ishtp_device *ishtp_dev, > spin_lock_irqsave(&ishtp_dev->device_list_lock, flags); > list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list, > device_link) { > + cl_device->fw_client = NULL; > if (warm_reset && cl_device->reference_count) > continue; >
On Wed, 2019-02-13 at 23:56 +0100, Jiri Kosina wrote: > On Tue, 12 Feb 2019, Hong Liu wrote: > > > When performing a warm reset in ishtp bus driver, the > > ishtp_cl_device > > will not be removed, its fw_client still points to the already > > freed > > ishtp_device.fw_clients array. > > > > Later after driver finishing ishtp client enumeration, this > > dangling > > pointer may cause driver to bind the wrong ishtp_cl_device to the > > new > > client, causing wrong callback to be called for messages intended > > for > > the new client. > > > > This helps in development of firmware where frequent switching of > > firmwares is required without Linux reboot. > > > > Signed-off-by: Hong Liu <hong.liu@intel.com> > > Tested-by: Hongyan Song <hongyan.song@intel.com> > > Srinivas, could you please ack this series? Thanks. Just did. I looked at these patches before during our internal reviews. Thanks, Srinivas > > > --- > > drivers/hid/intel-ish-hid/ishtp/bus.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c > > b/drivers/hid/intel-ish-hid/ishtp/bus.c > > index 728dc6d4561a..a271d6d169b1 100644 > > --- a/drivers/hid/intel-ish-hid/ishtp/bus.c > > +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c > > @@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl) > > spin_lock_irqsave(&cl->dev->device_list_lock, flags); > > list_for_each_entry(cl_device, &cl->dev->device_list, > > device_link) { > > - if (cl_device->fw_client->client_id == cl- > > >fw_client_id) { > > + if (cl_device->fw_client && > > + cl_device->fw_client->client_id == cl- > > >fw_client_id) { > > cl->device = cl_device; > > rv = 0; > > break; > > @@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct > > ishtp_device *ishtp_dev, > > spin_lock_irqsave(&ishtp_dev->device_list_lock, flags); > > list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list, > > device_link) { > > + cl_device->fw_client = NULL; > > if (warm_reset && cl_device->reference_count) > > continue; > > > > -- > > 2.20.1 > > > >
I've now applied the whole series. Thanks,
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c index 728dc6d4561a..a271d6d169b1 100644 --- a/drivers/hid/intel-ish-hid/ishtp/bus.c +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c @@ -675,7 +675,8 @@ int ishtp_cl_device_bind(struct ishtp_cl *cl) spin_lock_irqsave(&cl->dev->device_list_lock, flags); list_for_each_entry(cl_device, &cl->dev->device_list, device_link) { - if (cl_device->fw_client->client_id == cl->fw_client_id) { + if (cl_device->fw_client && + cl_device->fw_client->client_id == cl->fw_client_id) { cl->device = cl_device; rv = 0; break; @@ -735,6 +736,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev, spin_lock_irqsave(&ishtp_dev->device_list_lock, flags); list_for_each_entry_safe(cl_device, n, &ishtp_dev->device_list, device_link) { + cl_device->fw_client = NULL; if (warm_reset && cl_device->reference_count) continue;