| Message ID | 1559079417-32039-1-git-send-email-hyungwoo.yang@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] HID: intel-ish-hid: fix wrong type conversion | expand |
On Tue, 2019-05-28 at 14:36 -0700, Hyungwoo Yang wrote: > "struct device" is embedded in "struct ishtp_cl_device". > so the conversion should be done by container_of() method. Which tree this patch is going to? You can't even apply on the mainline tree (5.2-rc2). Also you will not be able to compile even if you address the conflict (The patch ordering is wrong). What was symptom or problem you try to address? Is there any crash or bug occurred? Does it happen with the mainline kernel? > > Change-Id: Idcbafe724e216ee6275f9d1e35a3b79cee5ce88c This tells me that you are trying to fix some Chrome issue. Don't include these tags for mainline kernel. Thanks, Srinivas > Signed-off-by: Hyungwoo Yang <hyungwoo.yang@intel.com> > --- > drivers/hid/intel-ish-hid/ishtp-hid-client.c | 4 ++-- > drivers/platform/chrome/cros_ec_ishtp.c | 4 ++-- > include/linux/intel-ish-client-if.h | 1 + > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c > b/drivers/hid/intel-ish-hid/ishtp-hid-client.c > index 56777a4..19102a3 100644 > --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c > +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c > @@ -899,7 +899,7 @@ static int hid_ishtp_cl_reset(struct > ishtp_cl_device *cl_device) > */ > static int hid_ishtp_cl_suspend(struct device *device) > { > - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); > + struct ishtp_cl_device *cl_device = > ishtp_dev_to_cl_device(device); > struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device); > struct ishtp_cl_data *client_data = > ishtp_get_client_data(hid_ishtp_cl); > > @@ -920,7 +920,7 @@ static int hid_ishtp_cl_suspend(struct device > *device) > */ > static int hid_ishtp_cl_resume(struct device *device) > { > - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); > + struct ishtp_cl_device *cl_device = > ishtp_dev_to_cl_device(device); > struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device); > struct ishtp_cl_data *client_data = > ishtp_get_client_data(hid_ishtp_cl); > > diff --git a/drivers/platform/chrome/cros_ec_ishtp.c > b/drivers/platform/chrome/cros_ec_ishtp.c > index e504d25..430731c 100644 > --- a/drivers/platform/chrome/cros_ec_ishtp.c > +++ b/drivers/platform/chrome/cros_ec_ishtp.c > @@ -707,7 +707,7 @@ static int cros_ec_ishtp_reset(struct > ishtp_cl_device *cl_device) > */ > static int __maybe_unused cros_ec_ishtp_suspend(struct device > *device) > { > - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); > + struct ishtp_cl_device *cl_device = > ishtp_dev_to_cl_device(device); > struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device); > struct ishtp_cl_data *client_data = > ishtp_get_client_data(cros_ish_cl); > > @@ -722,7 +722,7 @@ static int __maybe_unused > cros_ec_ishtp_suspend(struct device *device) > */ > static int __maybe_unused cros_ec_ishtp_resume(struct device > *device) > { > - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); > + struct ishtp_cl_device *cl_device = > ishtp_dev_to_cl_device(device); > struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device); > struct ishtp_cl_data *client_data = > ishtp_get_client_data(cros_ish_cl); > > diff --git a/include/linux/intel-ish-client-if.h > b/include/linux/intel-ish-client-if.h > index 16255c2..0d6b4bc 100644 > --- a/include/linux/intel-ish-client-if.h > +++ b/include/linux/intel-ish-client-if.h > @@ -103,6 +103,7 @@ int ishtp_register_event_cb(struct > ishtp_cl_device *device, > void ishtp_get_device(struct ishtp_cl_device *cl_dev); > void ishtp_set_drvdata(struct ishtp_cl_device *cl_device, void > *data); > void *ishtp_get_drvdata(struct ishtp_cl_device *cl_device); > +struct ishtp_cl_device *ishtp_dev_to_cl_device(struct device *dev); > int ishtp_register_event_cb(struct ishtp_cl_device *device, > void (*read_cb)(struct ishtp_cl_device > *)); > struct ishtp_fw_client *ishtp_fw_cl_get_client(struct > ishtp_device *dev,
> On Tue, 2019-05-28 at 14:36 -0700, Hyungwoo Yang wrote: > > "struct device" is embedded in "struct ishtp_cl_device". > > so the conversion should be done by container_of() method. > Which tree this patch is going to? You can't even apply on the mainline tree (5.2-rc2). Also you will not be able to compile even if you address the conflict (The patch ordering is wrong). Oh.. I wasn't careful to split the patch after testing. I've corrected. > > What was symptom or problem you try to address? Is there any crash or bug occurred? Does it happen with the mainline kernel? I've added the detail in commit message in v2. Basically due to wrong usage of driver_data of ishtp client device, we see kernel crash. Currently driver_data is set by bus driver which is wrong since driver_data should be owned by corresponding device driver. Right now, we see kernel crash during suspend() of cros_ec_ishtp. Yes, it happens with the mainline kernel since cros_ec_ishtp is already upstreamed. > > > > > > Change-Id: Idcbafe724e216ee6275f9d1e35a3b79cee5ce88c > This tells me that you are trying to fix some Chrome issue. Don't include these tags for mainline kernel. done in v2. Thank you. > > Thanks, > Srinivas Thanks, Hyungwoo
On Wed, 2019-05-29 at 07:21 +0000, Yang, Hyungwoo wrote: > > On Tue, 2019-05-28 at 14:36 -0700, Hyungwoo Yang wrote: > > > "struct device" is embedded in "struct ishtp_cl_device". > > > so the conversion should be done by container_of() method. > > > > Which tree this patch is going to? You can't even apply on the > > mainline tree (5.2-rc2). Also you will not be able to compile even > > if you address the conflict (The patch ordering is wrong). > > Oh.. I wasn't careful to split the patch after testing. I've > corrected. > > > > > What was symptom or problem you try to address? Is there any crash > > or bug occurred? Does it happen with the mainline kernel? > > I've added the detail in commit message in v2. Basically due to wrong > usage of driver_data of ishtp client device, we see kernel crash. > Currently driver_data is set by bus driver which is wrong since > driver_data should be owned by corresponding device driver. Right > now, we see kernel crash during suspend() of cros_ec_ishtp. Yes, it > happens with the mainline kernel since cros_ec_ishtp is already > upstreamed. Technically this driver is not mainline. It will go in 5.3. The problem is cros_ec ish driver is overriding driver_data " client_data->ec_dev = ec_dev; dev->driver_data = ec_dev; " The client drivers own the driver data in its "struct ishtp_cl_device *" not the struct device *. As far as I can see the purpose of this is to get device pointer for debug purpose only. I think you can remove the dev->driver_data assignment and simply replace dev_*(dev, to dev_*(ec_dev->dev, Thanks, Srinivas > > > > > > > > > > > Change-Id: Idcbafe724e216ee6275f9d1e35a3b79cee5ce88c > > > > This tells me that you are trying to fix some Chrome issue. Don't > > include these tags for mainline kernel. > > done in v2. Thank you. > > > > > Thanks, > > Srinivas > > Thanks, > Hyungwoo
> On Wed, 2019-05-29 at 07:21 +0000, Yang, Hyungwoo wrote: > > > On Tue, 2019-05-28 at 14:36 -0700, Hyungwoo Yang wrote: > > > > > > What was symptom or problem you try to address? Is there any crash > > > or bug occurred? Does it happen with the mainline kernel? > > > > I've added the detail in commit message in v2. Basically due to wrong > > usage of driver_data of ishtp client device, we see kernel crash. > > Currently driver_data is set by bus driver which is wrong since > > driver_data should be owned by corresponding device driver. Right now, > > we see kernel crash during suspend() of cros_ec_ishtp. Yes, it happens > > with the mainline kernel since cros_ec_ishtp is already upstreamed. > Technically this driver is not mainline. It will go in 5.3. > > The problem is cros_ec ish driver is overriding driver_data " > client_data->ec_dev = ec_dev; > dev->driver_data = ec_dev; > " > The client drivers own the driver data in its "struct ishtp_cl_device *" not the struct device *. No. still driver_data in "struct device" should be owned by its device driver. So there's no problem here since cros_ec_ish driver is owner of the device. > > As far as I can see the purpose of this is to get device pointer for debug purpose only. It's not for debug purpose and most importantly driver_data in "struct device" is used by its child in ec_device_probe() So wrong usage of driver_data should be corrected. > > I think you can remove the dev->driver_data assignment and simply replace > > dev_*(dev, > > to > dev_*(ec_dev->dev, > > Thanks, > Srinivas Thanks, Hyungwoo
On Wed, 2019-05-29 at 19:03 +0000, Yang, Hyungwoo wrote: > > On Wed, 2019-05-29 at 07:21 +0000, Yang, Hyungwoo wrote: > > > > On Tue, 2019-05-28 at 14:36 -0700, Hyungwoo Yang wrote: > > > > > > > > What was symptom or problem you try to address? Is there any > > > > crash > > > > or bug occurred? Does it happen with the mainline kernel? > > > > > > I've added the detail in commit message in v2. Basically due to > > > wrong > > > usage of driver_data of ishtp client device, we see kernel crash. > > > Currently driver_data is set by bus driver which is wrong since > > > driver_data should be owned by corresponding device driver. Right > > > now, > > > we see kernel crash during suspend() of cros_ec_ishtp. Yes, it > > > happens > > > with the mainline kernel since cros_ec_ishtp is already > > > upstreamed. > > > > Technically this driver is not mainline. It will go in 5.3. > > > > The problem is cros_ec ish driver is overriding driver_data " > > client_data->ec_dev = ec_dev; > > dev->driver_data = ec_dev; > > " > > The client drivers own the driver data in its "struct > > ishtp_cl_device *" not the struct device *. > > No. still driver_data in "struct device" should be owned by its > device driver. So there's no problem here since cros_ec_ish driver is > owner of the device. I don't know how the driver was submitted without suspend/resume test. Spilt the patch in this for bisect and the cros-ec is not in 5.2 yet. The first patch, is the combination of patch 1 and patch 2 excluding cros-ec changes. The second patch for cros-ec only using the new API. Thanks, Srinivas > > > > > As far as I can see the purpose of this is to get device pointer > > for debug purpose only. > > It's not for debug purpose and most importantly driver_data in > "struct device" is used by its child in ec_device_probe() > So wrong usage of driver_data should be corrected. > > > > > I think you can remove the dev->driver_data assignment and simply > > replace > > > > dev_*(dev, > > > > to > > dev_*(ec_dev->dev, > > > > Thanks, > > Srinivas > > Thanks, > Hyungwoo
On Wed, 2019-05-29 at 12:38 -0700, Srinivas Pandruvada wrote: > On Wed, 2019-05-29 at 19:03 +0000, Yang, Hyungwoo wrote: > > > On Wed, 2019-05-29 at 07:21 +0000, Yang, Hyungwoo wrote: > > > > > On Tue, 2019-05-28 at 14:36 -0700, Hyungwoo Yang wrote: > > > > > > > > > > What was symptom or problem you try to address? Is there any > > > > > crash > > > > > or bug occurred? Does it happen with the mainline kernel? > > > > > > > > I've added the detail in commit message in v2. Basically due to > > > > wrong > > > > usage of driver_data of ishtp client device, we see kernel > > > > crash. > > > > Currently driver_data is set by bus driver which is wrong > > > > since > > > > driver_data should be owned by corresponding device driver. > > > > Right > > > > now, > > > > we see kernel crash during suspend() of cros_ec_ishtp. Yes, it > > > > happens > > > > with the mainline kernel since cros_ec_ishtp is already > > > > upstreamed. > > > > > > Technically this driver is not mainline. It will go in 5.3. > > > > > > The problem is cros_ec ish driver is overriding driver_data " > > > client_data->ec_dev = ec_dev; > > > dev->driver_data = ec_dev; > > > " > > > The client drivers own the driver data in its "struct > > > ishtp_cl_device *" not the struct device *. > > > > No. still driver_data in "struct device" should be owned by its > > device driver. So there's no problem here since cros_ec_ish driver > > is > > owner of the device. > > I don't know how the driver was submitted without suspend/resume > test. > > Spilt the patch in this for bisect and the cros-ec is not in 5.2 yet. > > The first patch, is the combination of patch 1 and patch 2 excluding > cros-ec changes. > > The second patch for cros-ec only using the new API. Also run ./scripts/get_maintainer.pl on the patches to get maintainers/mailing list to send/copy. This patch probably needs to go along with cros-ec drivers pull request. Thanks, Srinivas > > Thanks, > Srinivas > > > > > > > > > As far as I can see the purpose of this is to get device pointer > > > for debug purpose only. > > > > It's not for debug purpose and most importantly driver_data in > > "struct device" is used by its child in ec_device_probe() > > So wrong usage of driver_data should be corrected. > > > > > > > > > > > I think you can remove the dev->driver_data assignment and simply > > > replace > > > > > > dev_*(dev, > > > > > > to > > > dev_*(ec_dev->dev, > > > > > > Thanks, > > > Srinivas > > > > Thanks, > > Hyungwoo
> On Wed, 2019-05-29 at 12:38 -0700, Srinivas Pandruvada wrote: > > On Wed, 2019-05-29 at 19:03 +0000, Yang, Hyungwoo wrote: > > > > > On Wed, 2019-05-29 at 07:21 +0000, Yang, Hyungwoo wrote: > > > > > > > On Tue, 2019-05-28 at 14:36 -0700, Hyungwoo Yang wrote: > > > > > > > > > > > > What was symptom or problem you try to address? Is there any > > > > > > crash or bug occurred? Does it happen with the mainline > > > > > > kernel? > > > > > > > > > > I've added the detail in commit message in v2. Basically due to > > > > > wrong usage of driver_data of ishtp client device, we see kernel > > > > > crash. > > > > > Currently driver_data is set by bus driver which is wrong since > > > > > driver_data should be owned by corresponding device driver. > > > > > Right > > > > > now, > > > > > we see kernel crash during suspend() of cros_ec_ishtp. Yes, it > > > > > happens with the mainline kernel since cros_ec_ishtp is already > > > > > upstreamed. > > > > > > > > Technically this driver is not mainline. It will go in 5.3. > > > > > > > > The problem is cros_ec ish driver is overriding driver_data " > > > > client_data->ec_dev = ec_dev; > > > > dev->driver_data = ec_dev; > > > > " > > > > The client drivers own the driver data in its "struct > > > > ishtp_cl_device *" not the struct device *. > > > > > > No. still driver_data in "struct device" should be owned by its > > > device driver. So there's no problem here since cros_ec_ish driver > > > is owner of the device. > > > > I don't know how the driver was submitted without suspend/resume test. > > > > Spilt the patch in this for bisect and the cros-ec is not in 5.2 yet. > > > > The first patch, is the combination of patch 1 and patch 2 excluding > > cros-ec changes. > > > > The second patch for cros-ec only using the new API. > > Also run > ./scripts/get_maintainer.pl on the patches > > to get maintainers/mailing list to send/copy. This patch probably needs to go along with cros-ec drivers pull request. Thank you, there was some mistake in sending patches but anyway like you said, I've submitted a patch combining patch1 and patch2 without cros-ec-ishtp. and also submitted one for cros-ec-ishtp. Thanks, Hyungwoo
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c index 56777a4..19102a3 100644 --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c @@ -899,7 +899,7 @@ static int hid_ishtp_cl_reset(struct ishtp_cl_device *cl_device) */ static int hid_ishtp_cl_suspend(struct device *device) { - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); + struct ishtp_cl_device *cl_device = ishtp_dev_to_cl_device(device); struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device); struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl); @@ -920,7 +920,7 @@ static int hid_ishtp_cl_suspend(struct device *device) */ static int hid_ishtp_cl_resume(struct device *device) { - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); + struct ishtp_cl_device *cl_device = ishtp_dev_to_cl_device(device); struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device); struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl); diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c index e504d25..430731c 100644 --- a/drivers/platform/chrome/cros_ec_ishtp.c +++ b/drivers/platform/chrome/cros_ec_ishtp.c @@ -707,7 +707,7 @@ static int cros_ec_ishtp_reset(struct ishtp_cl_device *cl_device) */ static int __maybe_unused cros_ec_ishtp_suspend(struct device *device) { - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); + struct ishtp_cl_device *cl_device = ishtp_dev_to_cl_device(device); struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device); struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl); @@ -722,7 +722,7 @@ static int __maybe_unused cros_ec_ishtp_suspend(struct device *device) */ static int __maybe_unused cros_ec_ishtp_resume(struct device *device) { - struct ishtp_cl_device *cl_device = dev_get_drvdata(device); + struct ishtp_cl_device *cl_device = ishtp_dev_to_cl_device(device); struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device); struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl); diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h index 16255c2..0d6b4bc 100644 --- a/include/linux/intel-ish-client-if.h +++ b/include/linux/intel-ish-client-if.h @@ -103,6 +103,7 @@ int ishtp_register_event_cb(struct ishtp_cl_device *device, void ishtp_get_device(struct ishtp_cl_device *cl_dev); void ishtp_set_drvdata(struct ishtp_cl_device *cl_device, void *data); void *ishtp_get_drvdata(struct ishtp_cl_device *cl_device); +struct ishtp_cl_device *ishtp_dev_to_cl_device(struct device *dev); int ishtp_register_event_cb(struct ishtp_cl_device *device, void (*read_cb)(struct ishtp_cl_device *)); struct ishtp_fw_client *ishtp_fw_cl_get_client(struct ishtp_device *dev,
"struct device" is embedded in "struct ishtp_cl_device". so the conversion should be done by container_of() method. Change-Id: Idcbafe724e216ee6275f9d1e35a3b79cee5ce88c Signed-off-by: Hyungwoo Yang <hyungwoo.yang@intel.com> --- drivers/hid/intel-ish-hid/ishtp-hid-client.c | 4 ++-- drivers/platform/chrome/cros_ec_ishtp.c | 4 ++-- include/linux/intel-ish-client-if.h | 1 + 3 files changed, 5 insertions(+), 4 deletions(-)