| Message ID | 20240904123607.3407364-2-lizetao1@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | HID: convert to devm_hid_hw_start_and_open() | expand |
On Sep 04 2024, Li Zetao wrote: > By adding a custom action to the device, it can bind the hid resource > to the hid_device life cycle. The framework automatically close and stop > the hid resources before hid_device is released, and the users do not > need to pay attention to the timing of hid resource release. > > Signed-off-by: Li Zetao <lizetao1@huawei.com> > --- > drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/linux/hid.h | 2 ++ > 2 files changed, 42 insertions(+) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 30de92d0bf0f..71143c0a4a02 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev) > } > EXPORT_SYMBOL_GPL(hid_hw_close); > > +static void hid_hw_close_and_stop(void *data) > +{ > + struct hid_device *hdev = data; > + > + hid_hw_close(hdev); > + hid_hw_stop(hdev); > +} > + > +/** > + * devm_hid_hw_start_and_open - manage hid resources through custom action > + * > + * @hdev: hid device > + * @connect_mask: which outputs to connect, see HID_CONNECT_* > + * > + * Bind the hid resource to the hid_device life cycle and register > + * an action to release the hid resource. The users do not need to > + * pay attention to the release of hid. The only problem I see here is that this API is not completely "safe", in the sense that a driver using it can not use manual kzalloc/kfree. It is obvious, but probably not so much while looking at the comments from Guenter where he asked you to also remove the .remove() callback. So in the docs, we should probably state that if the .remove() is any different than "hid_hw_close(hdev);hid_hw_stop(hdev);", care should be use with that function. Cheers, Benjamin > + */ > + > +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask) > +{ > + int ret; > + > + ret = hid_hw_start(hdev, connect_mask); > + if (ret) { > + hid_err(hdev, "hw start failed with %d\n", ret); > + return ret; > + } > + > + ret = hid_hw_open(hdev); > + if (ret) { > + hid_err(hdev, "hw open failed with %d\n", ret); > + hid_hw_stop(hdev); > + return ret; > + } > + > + return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev); > +} > +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open); > + > /** > * hid_hw_request - send report request to device > * > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 121d5b8bc867..0ce217aa5f62 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev, > void hid_hw_stop(struct hid_device *hdev); > int __must_check hid_hw_open(struct hid_device *hdev); > void hid_hw_close(struct hid_device *hdev); > +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev, > + unsigned int connect_mask); > void hid_hw_request(struct hid_device *hdev, > struct hid_report *report, enum hid_class_request reqtype); > int __hid_hw_raw_request(struct hid_device *hdev, > -- > 2.34.1 >
Hi, 在 2024/9/5 20:53, Benjamin Tissoires 写道: > On Sep 04 2024, Li Zetao wrote: >> By adding a custom action to the device, it can bind the hid resource >> to the hid_device life cycle. The framework automatically close and stop >> the hid resources before hid_device is released, and the users do not >> need to pay attention to the timing of hid resource release. >> >> Signed-off-by: Li Zetao <lizetao1@huawei.com> >> --- >> drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> include/linux/hid.h | 2 ++ >> 2 files changed, 42 insertions(+) >> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> index 30de92d0bf0f..71143c0a4a02 100644 >> --- a/drivers/hid/hid-core.c >> +++ b/drivers/hid/hid-core.c >> @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev) >> } >> EXPORT_SYMBOL_GPL(hid_hw_close); >> >> +static void hid_hw_close_and_stop(void *data) >> +{ >> + struct hid_device *hdev = data; >> + >> + hid_hw_close(hdev); >> + hid_hw_stop(hdev); >> +} >> + >> +/** >> + * devm_hid_hw_start_and_open - manage hid resources through custom action >> + * >> + * @hdev: hid device >> + * @connect_mask: which outputs to connect, see HID_CONNECT_* >> + * >> + * Bind the hid resource to the hid_device life cycle and register >> + * an action to release the hid resource. The users do not need to >> + * pay attention to the release of hid. > > The only problem I see here is that this API is not completely "safe", > in the sense that a driver using it can not use manual kzalloc/kfree. > It is obvious, but probably not so much while looking at the comments > from Guenter where he asked you to also remove the .remove() callback. > > So in the docs, we should probably state that if the .remove() is any > different than "hid_hw_close(hdev);hid_hw_stop(hdev);", care should be > use with that function.I'll add some comments to illustrate a scenario like this. > > Cheers, > Benjamin > >> + */ >> + >> +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask) >> +{ >> + int ret; >> + >> + ret = hid_hw_start(hdev, connect_mask); >> + if (ret) { >> + hid_err(hdev, "hw start failed with %d\n", ret); >> + return ret; >> + } >> + >> + ret = hid_hw_open(hdev); >> + if (ret) { >> + hid_err(hdev, "hw open failed with %d\n", ret); >> + hid_hw_stop(hdev); >> + return ret; >> + } >> + >> + return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev); >> +} >> +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open); >> + >> /** >> * hid_hw_request - send report request to device >> * >> diff --git a/include/linux/hid.h b/include/linux/hid.h >> index 121d5b8bc867..0ce217aa5f62 100644 >> --- a/include/linux/hid.h >> +++ b/include/linux/hid.h >> @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev, >> void hid_hw_stop(struct hid_device *hdev); >> int __must_check hid_hw_open(struct hid_device *hdev); >> void hid_hw_close(struct hid_device *hdev); >> +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev, >> + unsigned int connect_mask); >> void hid_hw_request(struct hid_device *hdev, >> struct hid_report *report, enum hid_class_request reqtype); >> int __hid_hw_raw_request(struct hid_device *hdev, >> -- >> 2.34.1 >> Thanks, Li Zetao.
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 30de92d0bf0f..71143c0a4a02 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev) } EXPORT_SYMBOL_GPL(hid_hw_close); +static void hid_hw_close_and_stop(void *data) +{ + struct hid_device *hdev = data; + + hid_hw_close(hdev); + hid_hw_stop(hdev); +} + +/** + * devm_hid_hw_start_and_open - manage hid resources through custom action + * + * @hdev: hid device + * @connect_mask: which outputs to connect, see HID_CONNECT_* + * + * Bind the hid resource to the hid_device life cycle and register + * an action to release the hid resource. The users do not need to + * pay attention to the release of hid. + */ + +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask) +{ + int ret; + + ret = hid_hw_start(hdev, connect_mask); + if (ret) { + hid_err(hdev, "hw start failed with %d\n", ret); + return ret; + } + + ret = hid_hw_open(hdev); + if (ret) { + hid_err(hdev, "hw open failed with %d\n", ret); + hid_hw_stop(hdev); + return ret; + } + + return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev); +} +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open); + /** * hid_hw_request - send report request to device * diff --git a/include/linux/hid.h b/include/linux/hid.h index 121d5b8bc867..0ce217aa5f62 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev, void hid_hw_stop(struct hid_device *hdev); int __must_check hid_hw_open(struct hid_device *hdev); void hid_hw_close(struct hid_device *hdev); +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev, + unsigned int connect_mask); void hid_hw_request(struct hid_device *hdev, struct hid_report *report, enum hid_class_request reqtype); int __hid_hw_raw_request(struct hid_device *hdev,
By adding a custom action to the device, it can bind the hid resource to the hid_device life cycle. The framework automatically close and stop the hid resources before hid_device is released, and the users do not need to pay attention to the timing of hid resource release. Signed-off-by: Li Zetao <lizetao1@huawei.com> --- drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/hid.h | 2 ++ 2 files changed, 42 insertions(+)