[PATCH 1/2] staging: fieldbus: anybus: Make remove callback return void

[PATCH 1/2] staging: fieldbus: anybus: 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 vio_driver::remove() return
void, too. All users already unconditionally return 0, this commit makes
it obvious that returning an error code is a bad idea.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/staging/fieldbus/anybuss/anybuss-client.h | 2 +-
 drivers/staging/fieldbus/anybuss/hms-profinet.c   | 3 +--
 drivers/staging/fieldbus/anybuss/host.c           | 3 ++-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fieldbus/anybuss/anybuss-client.h b/drivers/staging/fieldbus/anybuss/anybuss-client.h
index 8ee1f1baccf1..a219688006fe 100644
--- a/drivers/staging/fieldbus/anybuss/anybuss-client.h
+++ b/drivers/staging/fieldbus/anybuss/anybuss-client.h
@@ -32,7 +32,7 @@ struct anybuss_client {
 struct anybuss_client_driver {
 	struct device_driver driver;
 	int (*probe)(struct anybuss_client *adev);
-	int (*remove)(struct anybuss_client *adev);
+	void (*remove)(struct anybuss_client *adev);
 	u16 anybus_id;
 };
 
diff --git a/drivers/staging/fieldbus/anybuss/hms-profinet.c b/drivers/staging/fieldbus/anybuss/hms-profinet.c
index eca7d97b8e85..e691736a53f1 100644
--- a/drivers/staging/fieldbus/anybuss/hms-profinet.c
+++ b/drivers/staging/fieldbus/anybuss/hms-profinet.c
@@ -190,12 +190,11 @@ static int profinet_probe(struct anybuss_client *client)
 	return 0;
 }
 
-static int profinet_remove(struct anybuss_client *client)
+static void profinet_remove(struct anybuss_client *client)
 {
 	struct profi_priv *priv = anybuss_get_drvdata(client);
 
 	fieldbus_dev_unregister(&priv->fbdev);
-	return 0;
 }
 
 static struct anybuss_client_driver profinet_driver = {
diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c
index c97df91124a4..de6c287efa03 100644
--- a/drivers/staging/fieldbus/anybuss/host.c
+++ b/drivers/staging/fieldbus/anybuss/host.c
@@ -1194,7 +1194,8 @@ static int anybus_bus_remove(struct device *dev)
 		to_anybuss_client_driver(dev->driver);
 
 	if (adrv->remove)
-		return adrv->remove(to_anybuss_client(dev));
+		adrv->remove(to_anybuss_client(dev));
+
 	return 0;
 }
 

base-commit: d665ea6ea86c785760ee4bad4543dab3267ad074
-- 
2.30.2

[PATCH 2/2] staging: fieldbus: anybus: Refuse registering drivers without .probe()

[Uwe Kleine-König]

A driver without .probe() callback could never bind to a device because
anybus_bus_probe() returned an error for such a driver. So refuse to
register such a useless driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/staging/fieldbus/anybuss/host.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c
index de6c287efa03..0f730efe9a6d 100644
--- a/drivers/staging/fieldbus/anybuss/host.c
+++ b/drivers/staging/fieldbus/anybuss/host.c
@@ -1183,8 +1183,6 @@ static int anybus_bus_probe(struct device *dev)
 	struct anybuss_client *adev =
 		to_anybuss_client(dev);
 
-	if (!adrv->probe)
-		return -ENODEV;
 	return adrv->probe(adev);
 }
 
@@ -1208,6 +1206,9 @@ static struct bus_type anybus_bus = {
 
 int anybuss_client_driver_register(struct anybuss_client_driver *drv)
 {
+	if (!drv->probe)
+		return -ENODEV;
+
 	drv->driver.bus = &anybus_bus;
 	return driver_register(&drv->driver);
 }
-- 
2.30.2

Re: [PATCH 2/2] staging: fieldbus: anybus: Refuse registering drivers without .probe()

[Sven Van Asbroeck]

On Wed, May 5, 2021 at 4:29 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> A driver without .probe() callback could never bind to a device because
> anybus_bus_probe() returned an error for such a driver. So refuse to
> register such a useless driver.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Looks good to me. For the series:
Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>

Uwe, I'm glad you're submitting patches for fieldbus. Hope you find it
useful. This code is in staging mainly because only a single
company/developer is using it. So if you are actively using this, or
planning to use it, we can work towards taking this out of staging, if
you like.

Sven

Re: [PATCH 2/2] staging: fieldbus: anybus: Refuse registering drivers without .probe()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

Hello Sven,

On Wed, May 05, 2021 at 05:04:55PM -0400, Sven Van Asbroeck wrote:
> On Wed, May 5, 2021 at 4:29 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > A driver without .probe() callback could never bind to a device because
> > anybus_bus_probe() returned an error for such a driver. So refuse to
> > register such a useless driver.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Looks good to me. For the series:
> Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
> 
> Uwe, I'm glad you're submitting patches for fieldbus. Hope you find it
> useful. This code is in staging mainly because only a single
> company/developer is using it. So if you are actively using this, or
> planning to use it, we can work towards taking this out of staging, if
> you like.

It's not useful for me. My motivation is to make struct bus_type::remove
return void and this driver was just one of the offenders that
theoretically could return a non-zero value.

So sorry, I have no motivation to support getting this out of staging.

BTW: I just noticed there are occurences of anybuss and anybus in the
driver. Is only one of them right?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] staging: fieldbus: anybus: Make remove callback return void

[Fabio Aiuto]

Hello Uwe,

On Wed, May 05, 2021 at 10:29:22PM +0200, Uwe Kleine-König wrote:
> 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 vio_driver::remove() return
> void, too. All users already unconditionally return 0, this commit makes
> it obvious that returning an error code is a bad idea.

looks like that the commit description hardly matches what you changed
here. You changed the return type of handler remove() of struct
anybuss_client_driver. Are the latter and bus_type tied in some
fashion?


Why using :: notation?

> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/staging/fieldbus/anybuss/anybuss-client.h | 2 +-
>  drivers/staging/fieldbus/anybuss/hms-profinet.c   | 3 +--
>  drivers/staging/fieldbus/anybuss/host.c           | 3 ++-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/fieldbus/anybuss/anybuss-client.h b/drivers/staging/fieldbus/anybuss/anybuss-client.h
> index 8ee1f1baccf1..a219688006fe 100644
> --- a/drivers/staging/fieldbus/anybuss/anybuss-client.h
> +++ b/drivers/staging/fieldbus/anybuss/anybuss-client.h
> @@ -32,7 +32,7 @@ struct anybuss_client {
>  struct anybuss_client_driver {
>  	struct device_driver driver;
>  	int (*probe)(struct anybuss_client *adev);
> -	int (*remove)(struct anybuss_client *adev);
> +	void (*remove)(struct anybuss_client *adev);
>  	u16 anybus_id;
>  };
>  
> diff --git a/drivers/staging/fieldbus/anybuss/hms-profinet.c b/drivers/staging/fieldbus/anybuss/hms-profinet.c
> index eca7d97b8e85..e691736a53f1 100644
> --- a/drivers/staging/fieldbus/anybuss/hms-profinet.c
> +++ b/drivers/staging/fieldbus/anybuss/hms-profinet.c
> @@ -190,12 +190,11 @@ static int profinet_probe(struct anybuss_client *client)
>  	return 0;
>  }
>  
> -static int profinet_remove(struct anybuss_client *client)
> +static void profinet_remove(struct anybuss_client *client)
>  {
>  	struct profi_priv *priv = anybuss_get_drvdata(client);
>  
>  	fieldbus_dev_unregister(&priv->fbdev);
> -	return 0;
>  }
>  
>  static struct anybuss_client_driver profinet_driver = {
> diff --git a/drivers/staging/fieldbus/anybuss/host.c b/drivers/staging/fieldbus/anybuss/host.c
> index c97df91124a4..de6c287efa03 100644
> --- a/drivers/staging/fieldbus/anybuss/host.c
> +++ b/drivers/staging/fieldbus/anybuss/host.c
> @@ -1194,7 +1194,8 @@ static int anybus_bus_remove(struct device *dev)
>  		to_anybuss_client_driver(dev->driver);
>  
>  	if (adrv->remove)
> -		return adrv->remove(to_anybuss_client(dev));
> +		adrv->remove(to_anybuss_client(dev));
> +
>  	return 0;
>  }
>  
> 
> base-commit: d665ea6ea86c785760ee4bad4543dab3267ad074
> -- 
> 2.30.2
> 
> 

thank you,

fabio

Re: [PATCH 1/2] staging: fieldbus: anybus: Make remove callback return void

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Thu, May 06, 2021 at 08:48:44AM +0200, Fabio Aiuto wrote:
> Hello Uwe,
> 
> On Wed, May 05, 2021 at 10:29:22PM +0200, Uwe Kleine-König wrote:
> > 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 vio_driver::remove() return
> > void, too. All users already unconditionally return 0, this commit makes
> > it obvious that returning an error code is a bad idea.
> 
> looks like that the commit description hardly matches what you changed
> here. You changed the return type of handler remove() of struct
> anybuss_client_driver. Are the latter and bus_type tied in some
> fashion?

Oh, vio_driver leaked from my copy-and-paste template.

> Why using :: notation?

If you have a better suggestion I'm all ears.

Will resend with Sven's Reviewed-by and
s/vio_driver/anybuss_client_driver/

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] staging: fieldbus: anybus: Make remove callback return void

[Fabio Aiuto]

On Thu, May 06, 2021 at 09:52:13AM +0200, Uwe Kleine-König wrote:
> On Thu, May 06, 2021 at 08:48:44AM +0200, Fabio Aiuto wrote:
> > Hello Uwe,
> > 
> > On Wed, May 05, 2021 at 10:29:22PM +0200, Uwe Kleine-König wrote:
> > > 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 vio_driver::remove() return
> > > void, too. All users already unconditionally return 0, this commit makes
> > > it obvious that returning an error code is a bad idea.
> > 
> > looks like that the commit description hardly matches what you changed
> > here. You changed the return type of handler remove() of struct
> > anybuss_client_driver. Are the latter and bus_type tied in some
> > fashion?
> 
> Oh, vio_driver leaked from my copy-and-paste template.
> 
> > Why using :: notation?
> 
> If you have a better suggestion I'm all ears.

maybe a simple dot? '::' reminds c++ or rust, this patch fixes c code.
But I don't think it is a real issue, I was just wondering why you choose
'::'

> 
> Will resend with Sven's Reviewed-by and
> s/vio_driver/anybuss_client_driver/
> 
> Thanks
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

thank you,

fabio

Re: [PATCH 2/2] staging: fieldbus: anybus: Refuse registering drivers without .probe()

[Fabio Aiuto]

On Wed, May 05, 2021 at 05:04:55PM -0400, Sven Van Asbroeck wrote:
> On Wed, May 5, 2021 at 4:29 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > A driver without .probe() callback could never bind to a device because
> > anybus_bus_probe() returned an error for such a driver. So refuse to
> > register such a useless driver.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Looks good to me. For the series:
> Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
> 
> Uwe, I'm glad you're submitting patches for fieldbus. Hope you find it
> useful. This code is in staging mainly because only a single
> company/developer is using it. So if you are actively using this, or
> planning to use it, we can work towards taking this out of staging, if
> you like.
> 
> Sven
> 

Hi Sven,

I'm trying to make my way in linux kernel device driver development and
hopefully find a job soon. If the task 'moving fieldbus out of staging' will
give me opportunity to play with real hardware, and so gain more experience,
we can talk about it.

thank you,

fabio

Re: [PATCH 1/2] staging: fieldbus: anybus: Make remove callback return void

[Dan Carpenter]

On Thu, May 06, 2021 at 10:40:18AM +0200, Fabio Aiuto wrote:
> On Thu, May 06, 2021 at 09:52:13AM +0200, Uwe Kleine-König wrote:
> > On Thu, May 06, 2021 at 08:48:44AM +0200, Fabio Aiuto wrote:
> > > Hello Uwe,
> > > 
> > > On Wed, May 05, 2021 at 10:29:22PM +0200, Uwe Kleine-König wrote:
> > > > 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 vio_driver::remove() return
> > > > void, too. All users already unconditionally return 0, this commit makes
> > > > it obvious that returning an error code is a bad idea.
> > > 
> > > looks like that the commit description hardly matches what you changed
> > > here. You changed the return type of handler remove() of struct
> > > anybuss_client_driver. Are the latter and bus_type tied in some
> > > fashion?
> > 
> > Oh, vio_driver leaked from my copy-and-paste template.
> > 
> > > Why using :: notation?
> > 
> > If you have a better suggestion I'm all ears.
> 
> maybe a simple dot? '::' reminds c++ or rust, this patch fixes c code.
> But I don't think it is a real issue, I was just wondering why you choose
> '::'

In Smatch I use:

	(struct vio_driver)->remove

But in emails I normally put a () on the end of function names.

regards,
dan carpenter

Re: [PATCH 1/2] staging: fieldbus: anybus: Make remove callback return void

[Sven Van Asbroeck]

On Thu, May 6, 2021 at 3:52 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Will resend with Sven's Reviewed-by and
> s/vio_driver/anybuss_client_driver/

Greg KH has already taken care of it in his tree, so we're good to go.
Thanks Greg!
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=239db3ea2f37b75168ca5d0577a848e263669b89

Re: [PATCH 2/2] staging: fieldbus: anybus: Refuse registering drivers without .probe()

[Sven Van Asbroeck]

Hi Uwe,

On Thu, May 6, 2021 at 2:17 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> So sorry, I have no motivation to support getting this out of staging.

No worries, this subsystem would only be useful to you, if you were
running Linux on a Fieldbus client device. That is still quite niche -
on mainline.

>
> BTW: I just noticed there are occurences of anybuss and anybus in the
> driver. Is only one of them right?

There are various "flavours" of Anybus. One of them is Anybus-S, which
is the one implemented here. Support could be added for the other
flavours, but there is no use case right now. The reason why I went
with "anybuss_xxx" instead of "anybus_s_xxx" was purely visual. Code
such as this:

> void anybus_s_client_driver_unregister(struct anybuss_client_driver *drv)

looked really bad to me. Perhaps in hindsight, "anybus_s_xxx" would
have been the better choice, because quite a few people have
questioned the double-S. But hindsight is 20/20, and there is no good
incentive to change it now, except if Anybus-S suddenly became more
popular. Which is unlikely, it's now purely a legacy bus.

Re: [PATCH 2/2] staging: fieldbus: anybus: Refuse registering drivers without .probe()

[Sven Van Asbroeck]

Hi Fabio,

On Thu, May 6, 2021 at 4:49 AM Fabio Aiuto <fabioaiuto83@gmail.com> wrote:
>
> I'm trying to make my way in linux kernel device driver development and
> hopefully find a job soon. If the task 'moving fieldbus out of staging' will
> give me opportunity to play with real hardware, and so gain more experience,
> we can talk about it.

Thank you so much for the offer, I really appreciate it !

The fieldbus subsystem sits in staging mainly because it's in use by
only a single company. Greg KH was absolutely right to insist that
this had to go into staging: a subsystem is different from a simple
driver, and can only thrive if it's actively used by a wide community.
You can see this in the subsystem's TODO:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fieldbus/TODO?h=v5.12

In short, this cannot be taken out of staging by "pure technical
work". Except if you had a fieldbus client device on your hands, and
you wanted to talk to it through this subsystem.

Sven

Re: [PATCH 2/2] staging: fieldbus: anybus: Refuse registering drivers without .probe()

[Fabio Aiuto]

On Thu, May 06, 2021 at 09:54:06AM -0400, Sven Van Asbroeck wrote:
> Hi Fabio,
> 
> On Thu, May 6, 2021 at 4:49 AM Fabio Aiuto <fabioaiuto83@gmail.com> wrote:
> >
> > I'm trying to make my way in linux kernel device driver development and
> > hopefully find a job soon. If the task 'moving fieldbus out of staging' will
> > give me opportunity to play with real hardware, and so gain more experience,
> > we can talk about it.
> 
> Thank you so much for the offer, I really appreciate it !
> 
> The fieldbus subsystem sits in staging mainly because it's in use by
> only a single company. Greg KH was absolutely right to insist that
> this had to go into staging: a subsystem is different from a simple
> driver, and can only thrive if it's actively used by a wide community.
> You can see this in the subsystem's TODO:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fieldbus/TODO?h=v5.12

for this TODO, one should identify first target people who
could be interested in Fieldbus.

> 
> In short, this cannot be taken out of staging by "pure technical
> work". Except if you had a fieldbus client device on your hands, and
> you wanted to talk to it through this subsystem.

no I don't, these seems to be industrial devices and I don't
know how could I bring home such stuff.

> 
> Sven

thank you for your answer Sven,

fabio