[PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[Uwe Kleine-König]

A remove callback is only ever called for a bound device. So there is no
need to check for device or driver being NULL.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/hid/intel-ish-hid/ishtp/bus.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index bba29cd36d29..ccd54f244503 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -257,17 +257,13 @@ static int ishtp_cl_bus_match(struct device *dev, struct device_driver *drv)
 static int ishtp_cl_device_remove(struct device *dev)
 {
 	struct ishtp_cl_device *device = to_ishtp_cl_device(dev);
-	struct ishtp_cl_driver *driver;
-
-	if (!device || !dev->driver)
-		return 0;
+	struct ishtp_cl_driver *driver = to_ishtp_cl_driver(dev->driver);
 
 	if (device->event_cb) {
 		device->event_cb = NULL;
 		cancel_work_sync(&device->event_work);
 	}
 
-	driver = to_ishtp_cl_driver(dev->driver);
 	if (!driver->remove) {
 		dev->driver = NULL;
 

base-commit: 5c8fe583cce542aa0b84adc939ce85293de36e5e
-- 
2.29.2

[PATCH v1 2/3] HID: intel-ish-hid: Simplify logic in ishtp_cl_device_remove()

[Uwe Kleine-König]

There is only a single change in behavior: Now dev->driver isn't modified.
Assigning to this variable is in the domain of the driver core only. (And
it's done in __device_release_driver shortly after bus->remove() (i.e
ishtp_cl_device_remove() here) returns.)

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/hid/intel-ish-hid/ishtp/bus.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index ccd54f244503..7f36ce6187a1 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -258,19 +258,17 @@ static int ishtp_cl_device_remove(struct device *dev)
 {
 	struct ishtp_cl_device *device = to_ishtp_cl_device(dev);
 	struct ishtp_cl_driver *driver = to_ishtp_cl_driver(dev->driver);
+	int ret = 0;
 
 	if (device->event_cb) {
 		device->event_cb = NULL;
 		cancel_work_sync(&device->event_work);
 	}
 
-	if (!driver->remove) {
-		dev->driver = NULL;
+	if (driver->remove)
+		ret = driver->remove(device);
 
-		return 0;
-	}
-
-	return driver->remove(device);
+	return ret;
 }
 
 /**
-- 
2.29.2

[PATCH v1 3/3] HID: intel-ish-hid: Make remove callback return void

[Uwe Kleine-König]

The driver core ignores the return value of struct bus_type::remove()
because there is only little that can be done. To simplify the quest to
make this function return void, let struct ishtp_cl_driver::remove() return
void, too. All users already unconditionally return 0, this commit makes
it obvious that returning an error value is a bad idea.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/hid/intel-ish-hid/ishtp-fw-loader.c  | 4 +---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 4 +---
 drivers/hid/intel-ish-hid/ishtp/bus.c        | 5 ++---
 drivers/platform/chrome/cros_ec_ishtp.c      | 4 +---
 include/linux/intel-ish-client-if.h          | 2 +-
 5 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
index 6cf59fd26ad7..edb0bd084c27 100644
--- a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
+++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
@@ -1015,7 +1015,7 @@ static int loader_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
  *
  * Return: 0
  */
-static int loader_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
+static void loader_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
 {
 	struct ishtp_cl_data *client_data;
 	struct ishtp_cl	*loader_ishtp_cl = ishtp_get_drvdata(cl_device);
@@ -1032,8 +1032,6 @@ static int loader_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
 	cancel_work_sync(&client_data->work_ishtp_reset);
 	loader_deinit(loader_ishtp_cl);
 	ishtp_put_device(cl_device);
-
-	return 0;
 }
 
 /**
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 6ba944b40fdb..0f1b5283bab4 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -838,7 +838,7 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
  *
  * Return: 0
  */
-static int hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
+static void hid_ishtp_cl_remove(struct ishtp_cl_device *cl_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);
@@ -856,8 +856,6 @@ static int hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
 	hid_ishtp_cl = NULL;
 
 	client_data->num_hid_devices = 0;
-
-	return 0;
 }
 
 /**
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 7f36ce6187a1..ffc9ce5c86ee 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -258,7 +258,6 @@ static int ishtp_cl_device_remove(struct device *dev)
 {
 	struct ishtp_cl_device *device = to_ishtp_cl_device(dev);
 	struct ishtp_cl_driver *driver = to_ishtp_cl_driver(dev->driver);
-	int ret = 0;
 
 	if (device->event_cb) {
 		device->event_cb = NULL;
@@ -266,9 +265,9 @@ static int ishtp_cl_device_remove(struct device *dev)
 	}
 
 	if (driver->remove)
-		ret = driver->remove(device);
+		driver->remove(device);
 
-	return ret;
+	return 0;
 }
 
 /**
diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c
index 81364029af36..bd80173b33a2 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_probe(struct ishtp_cl_device *cl_device)
  *
  * Return: 0
  */
-static int cros_ec_ishtp_remove(struct ishtp_cl_device *cl_device)
+static void cros_ec_ishtp_remove(struct ishtp_cl_device *cl_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);
@@ -716,8 +716,6 @@ static int cros_ec_ishtp_remove(struct ishtp_cl_device *cl_device)
 	cancel_work_sync(&client_data->work_ec_evt);
 	cros_ish_deinit(cros_ish_cl);
 	ishtp_put_device(cl_device);
-
-	return 0;
 }
 
 /**
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index 0d6b4bc191c5..94669e21dc8b 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -36,7 +36,7 @@ struct ishtp_cl_driver {
 	const char *name;
 	const guid_t *guid;
 	int (*probe)(struct ishtp_cl_device *dev);
-	int (*remove)(struct ishtp_cl_device *dev);
+	void (*remove)(struct ishtp_cl_device *dev);
 	int (*reset)(struct ishtp_cl_device *dev);
 	const struct dev_pm_ops *pm;
 };
-- 
2.29.2

Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[Jiri Kosina]

On Sat, 6 Feb 2021, Uwe Kleine-König wrote:

> A remove callback is only ever called for a bound device. So there is no
> need to check for device or driver being NULL.

Srinivas, any objections to this patchset? The cleanups look good to me. 
Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[Srinivas Pandruvada]

On Mon, 2021-03-08 at 11:07 +0100, Jiri Kosina wrote:
> On Sat, 6 Feb 2021, Uwe Kleine-König wrote:
> 
> > A remove callback is only ever called for a bound device. So there
> > is no
> > need to check for device or driver being NULL.
> 
> Srinivas, any objections to this patchset? The cleanups look good to
> me. 
Sorry, I missed this series.
No objection for taking these patches.

Thanks,
Srinivas


> Thanks,
> 

Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[Jiri Kosina]

On Mon, 8 Mar 2021, Srinivas Pandruvada wrote:

> > > A remove callback is only ever called for a bound device. So there
> > > is no
> > > need to check for device or driver being NULL.
> > 
> > Srinivas, any objections to this patchset? The cleanups look good to
> > me. 
> Sorry, I missed this series.
> No objection for taking these patches.

Thanks. Applied with your Acked-by:
If you disagree with that interpretation of your statement above, please 
holler :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[Srinivas Pandruvada]

On Mon, 2021-03-08 at 17:16 +0100, Jiri Kosina wrote:
> On Mon, 8 Mar 2021, Srinivas Pandruvada wrote:
> 
> > > > A remove callback is only ever called for a bound device. So
> > > > there
> > > > is no
> > > > need to check for device or driver being NULL.
> > > 
> > > Srinivas, any objections to this patchset? The cleanups look good
> > > to
> > > me. 
> > Sorry, I missed this series.
> > No objection for taking these patches.
> 
> Thanks. Applied with your Acked-by:
> If you disagree with that interpretation of your statement above,
> please 
> holler :)
I agree. 
For record:
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

> 
> Thanks,
> 

Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1184 bytes --]

Hello Jiri,

On 3/8/21 5:16 PM, Jiri Kosina wrote:
> On Mon, 8 Mar 2021, Srinivas Pandruvada wrote:
> 
>>>> A remove callback is only ever called for a bound device. So there
>>>> is no
>>>> need to check for device or driver being NULL.
>>>
>>> Srinivas, any objections to this patchset? The cleanups look good to
>>> me.
>> Sorry, I missed this series.
>> No objection for taking these patches.
> 
> Thanks. Applied with your Acked-by:
> If you disagree with that interpretation of your statement above, please
> holler :)

I expected these patches to go in during the 5.13 merge window, but they 
didn't. I found your pull request for 5.13 
(https://lore.kernel.org/lkml/nycvar.YFH.7.76.2104292151220.18270@cbobk.fhfr.pm/) 
and they were not included there even though the patches were in next 
since at least next-20210310. Looking at

	git log --oneline --cherry 
v5.13-rc1...dce6a0d56a7719efcad438f5c46a9d192fd36a89

(where dce.. was the tip of your for-next for next-20210506 (i.e. before 
5.13-rc1 was cut)) and it seems there are quite a few more commits that 
didn't make it into your pull request.

What am I missing?

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/3] HID: intel-ish-hid: Drop if block with an always false condition

[Jiri Kosina]

On Mon, 10 May 2021, Uwe Kleine-König wrote:

> I expected these patches to go in during the 5.13 merge window, but they
> didn't. I found your pull request for 5.13
> (https://lore.kernel.org/lkml/nycvar.YFH.7.76.2104292151220.18270@cbobk.fhfr.pm/)
> and they were not included there even though the patches were in next since at
> least next-20210310. Looking at
> 
> 	git log --oneline --cherry
> v5.13-rc1...dce6a0d56a7719efcad438f5c46a9d192fd36a89
> 
> (where dce.. was the tip of your for-next for next-20210506 (i.e. before
> 5.13-rc1 was cut)) and it seems there are quite a few more commits that didn't
> make it into your pull request.
> 
> What am I missing?

You are missing the fact that I am a halfwit and I screwed up the merge :) 

for-5.13/intel-ish branch by mistake didn't make it into final for-linus 
unfortunately, due to my mistake.

Thanks a lot for pointing it out, I will fix that up.

-- 
Jiri Kosina
SUSE Labs