[PATCH -next 1/2] xtensa: iss: fix handling error cases in iss_net_configure()

[PATCH -next 1/2] xtensa: iss: fix handling error cases in iss_net_configure()

[Yang Yingliang]

The 'pdev' and 'netdev' need be release in error cases of iss_net_configure().
In orde to fix this, add two labels for error case to release them.

Fixes: 7282bee78798 ("[PATCH] xtensa: Architecture support for Tensilica Xtensa Part 8")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 arch/xtensa/platforms/iss/network.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index fd84d4891758..5e475f7472e4 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -481,7 +481,7 @@ static int iss_net_configure(int index, char *init)
 	dev = alloc_etherdev(sizeof(*lp));
 	if (dev == NULL) {
 		pr_err("eth_configure: failed to allocate device\n");
-		return 1;
+		return -ENOMEM;
 	}
 
 	/* Initialize private element. */
@@ -509,7 +509,8 @@ static int iss_net_configure(int index, char *init)
 	if (!tuntap_probe(lp, index, init)) {
 		pr_err("%s: invalid arguments. Skipping device!\n",
 		       dev->name);
-		goto errout;
+		err = -EINVAL;
+		goto err_free_netdev;
 	}
 
 	pr_info("Netdevice %d (%pM)\n", index, dev->dev_addr);
@@ -517,7 +518,9 @@ static int iss_net_configure(int index, char *init)
 	/* sysfs register */
 
 	if (!driver_registered) {
-		platform_driver_register(&iss_net_driver);
+		err = platform_driver_register(&iss_net_driver);
+		if (err)
+			goto err_free_netdev;
 		driver_registered = 1;
 	}
 
@@ -527,7 +530,9 @@ static int iss_net_configure(int index, char *init)
 
 	lp->pdev.id = index;
 	lp->pdev.name = DRIVER_NAME;
-	platform_device_register(&lp->pdev);
+	err = platform_device_register(&lp->pdev);
+	if (err)
+		goto err_free_netdev;
 	SET_NETDEV_DEV(dev, &lp->pdev.dev);
 
 	dev->netdev_ops = &iss_netdev_ops;
@@ -541,18 +546,18 @@ static int iss_net_configure(int index, char *init)
 
 	if (err) {
 		pr_err("%s: error registering net device!\n", dev->name);
-		/* XXX: should we call ->remove() here? */
-		free_netdev(dev);
-		return 1;
+		goto err_unregister_device;
 	}
 
 	timer_setup(&lp->tl, iss_net_user_timer_expire, 0);
 
 	return 0;
 
-errout:
-	/* FIXME: unregister; free, etc.. */
-	return -EIO;
+err_unregister_device:
+	platform_device_unregister(&lp->pdev);
+err_free_netdev:
+	free_netdev(dev);
+	return err;
 }
 
 /* ------------------------------------------------------------------------- */
-- 
2.25.1

[PATCH -next 2/2] xtensa: iss: change the return type of iss_net_configure() to void

[Yang Yingliang]

Change the return type of iss_net_configure() to void, because it's not used.

Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 arch/xtensa/platforms/iss/network.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
index 5e475f7472e4..e38ff51ce833 100644
--- a/arch/xtensa/platforms/iss/network.c
+++ b/arch/xtensa/platforms/iss/network.c
@@ -472,16 +472,15 @@ static const struct net_device_ops iss_netdev_ops = {
 	.ndo_set_rx_mode	= iss_net_set_multicast_list,
 };
 
-static int iss_net_configure(int index, char *init)
+static void iss_net_configure(int index, char *init)
 {
 	struct net_device *dev;
 	struct iss_net_private *lp;
-	int err;
 
 	dev = alloc_etherdev(sizeof(*lp));
 	if (dev == NULL) {
 		pr_err("eth_configure: failed to allocate device\n");
-		return -ENOMEM;
+		return;
 	}
 
 	/* Initialize private element. */
@@ -509,7 +508,6 @@ static int iss_net_configure(int index, char *init)
 	if (!tuntap_probe(lp, index, init)) {
 		pr_err("%s: invalid arguments. Skipping device!\n",
 		       dev->name);
-		err = -EINVAL;
 		goto err_free_netdev;
 	}
 
@@ -518,8 +516,7 @@ static int iss_net_configure(int index, char *init)
 	/* sysfs register */
 
 	if (!driver_registered) {
-		err = platform_driver_register(&iss_net_driver);
-		if (err)
+		if (platform_driver_register(&iss_net_driver))
 			goto err_free_netdev;
 		driver_registered = 1;
 	}
@@ -530,8 +527,7 @@ static int iss_net_configure(int index, char *init)
 
 	lp->pdev.id = index;
 	lp->pdev.name = DRIVER_NAME;
-	err = platform_device_register(&lp->pdev);
-	if (err)
+	if (platform_device_register(&lp->pdev))
 		goto err_free_netdev;
 	SET_NETDEV_DEV(dev, &lp->pdev.dev);
 
@@ -541,23 +537,22 @@ static int iss_net_configure(int index, char *init)
 	dev->irq = -1;
 
 	rtnl_lock();
-	err = register_netdevice(dev);
-	rtnl_unlock();
-
-	if (err) {
+	if (register_netdevice(dev)) {
+		rtnl_unlock();
 		pr_err("%s: error registering net device!\n", dev->name);
 		goto err_unregister_device;
 	}
+	rtnl_unlock();
 
 	timer_setup(&lp->tl, iss_net_user_timer_expire, 0);
 
-	return 0;
+	return;
 
 err_unregister_device:
 	platform_device_unregister(&lp->pdev);
 err_free_netdev:
 	free_netdev(dev);
-	return err;
+	return;
 }
 
 /* ------------------------------------------------------------------------- */
-- 
2.25.1

Re: [PATCH -next 2/2] xtensa: iss: change the return type of iss_net_configure() to void

[Max Filippov]

Hi Yang,

On Tue, Jul 5, 2022 at 6:10 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
>
> Change the return type of iss_net_configure() to void, because it's not used.
>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  arch/xtensa/platforms/iss/network.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)

This change removes a lot of code that was added in the previous change.
Maybe fold both patches into one to avoid that pattern of adding and then
immediately removing code? Or, if you feel that changing the return type
of the iss_net_configure is an important separate step, maybe do it first?

> diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
> index 5e475f7472e4..e38ff51ce833 100644
> --- a/arch/xtensa/platforms/iss/network.c
> +++ b/arch/xtensa/platforms/iss/network.c
> @@ -472,16 +472,15 @@ static const struct net_device_ops iss_netdev_ops = {
>         .ndo_set_rx_mode        = iss_net_set_multicast_list,
>  };
>
> -static int iss_net_configure(int index, char *init)
> +static void iss_net_configure(int index, char *init)
>  {
>         struct net_device *dev;
>         struct iss_net_private *lp;
> -       int err;
>
>         dev = alloc_etherdev(sizeof(*lp));
>         if (dev == NULL) {
>                 pr_err("eth_configure: failed to allocate device\n");
> -               return -ENOMEM;
> +               return;
>         }
>
>         /* Initialize private element. */
> @@ -509,7 +508,6 @@ static int iss_net_configure(int index, char *init)
>         if (!tuntap_probe(lp, index, init)) {
>                 pr_err("%s: invalid arguments. Skipping device!\n",
>                        dev->name);
> -               err = -EINVAL;
>                 goto err_free_netdev;
>         }
>
> @@ -518,8 +516,7 @@ static int iss_net_configure(int index, char *init)
>         /* sysfs register */
>
>         if (!driver_registered) {
> -               err = platform_driver_register(&iss_net_driver);
> -               if (err)
> +               if (platform_driver_register(&iss_net_driver))
>                         goto err_free_netdev;
>                 driver_registered = 1;
>         }
> @@ -530,8 +527,7 @@ static int iss_net_configure(int index, char *init)
>
>         lp->pdev.id = index;
>         lp->pdev.name = DRIVER_NAME;
> -       err = platform_device_register(&lp->pdev);
> -       if (err)
> +       if (platform_device_register(&lp->pdev))
>                 goto err_free_netdev;
>         SET_NETDEV_DEV(dev, &lp->pdev.dev);
>
> @@ -541,23 +537,22 @@ static int iss_net_configure(int index, char *init)
>         dev->irq = -1;
>
>         rtnl_lock();
> -       err = register_netdevice(dev);
> -       rtnl_unlock();
> -
> -       if (err) {
> +       if (register_netdevice(dev)) {
> +               rtnl_unlock();
>                 pr_err("%s: error registering net device!\n", dev->name);
>                 goto err_unregister_device;
>         }
> +       rtnl_unlock();
>
>         timer_setup(&lp->tl, iss_net_user_timer_expire, 0);
>
> -       return 0;
> +       return;
>
>  err_unregister_device:
>         platform_device_unregister(&lp->pdev);
>  err_free_netdev:
>         free_netdev(dev);
> -       return err;
> +       return;

No need for 'return' at the end of the void function.

-- 
Thanks.
-- Max

Re: [PATCH -next 2/2] xtensa: iss: change the return type of iss_net_configure() to void

[Yang Yingliang]

Hi,

On 2022/7/5 23:59, Max Filippov wrote:
> Hi Yang,
>
> On Tue, Jul 5, 2022 at 6:10 AM Yang Yingliang <yangyingliang@huawei.com> wrote:
>> Change the return type of iss_net_configure() to void, because it's not used.
>>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>>   arch/xtensa/platforms/iss/network.c | 23 +++++++++--------------
>>   1 file changed, 9 insertions(+), 14 deletions(-)
> This change removes a lot of code that was added in the previous change.
> Maybe fold both patches into one to avoid that pattern of adding and then
> immediately removing code? Or, if you feel that changing the return type
> of the iss_net_configure is an important separate step, maybe do it first?
I will send a v2 with these changes in one patch.

Thanks,
Yang
>
>> diff --git a/arch/xtensa/platforms/iss/network.c b/arch/xtensa/platforms/iss/network.c
>> index 5e475f7472e4..e38ff51ce833 100644
>> --- a/arch/xtensa/platforms/iss/network.c
>> +++ b/arch/xtensa/platforms/iss/network.c
>> @@ -472,16 +472,15 @@ static const struct net_device_ops iss_netdev_ops = {
>>          .ndo_set_rx_mode        = iss_net_set_multicast_list,
>>   };
>>
>> -static int iss_net_configure(int index, char *init)
>> +static void iss_net_configure(int index, char *init)
>>   {
>>          struct net_device *dev;
>>          struct iss_net_private *lp;
>> -       int err;
>>
>>          dev = alloc_etherdev(sizeof(*lp));
>>          if (dev == NULL) {
>>                  pr_err("eth_configure: failed to allocate device\n");
>> -               return -ENOMEM;
>> +               return;
>>          }
>>
>>          /* Initialize private element. */
>> @@ -509,7 +508,6 @@ static int iss_net_configure(int index, char *init)
>>          if (!tuntap_probe(lp, index, init)) {
>>                  pr_err("%s: invalid arguments. Skipping device!\n",
>>                         dev->name);
>> -               err = -EINVAL;
>>                  goto err_free_netdev;
>>          }
>>
>> @@ -518,8 +516,7 @@ static int iss_net_configure(int index, char *init)
>>          /* sysfs register */
>>
>>          if (!driver_registered) {
>> -               err = platform_driver_register(&iss_net_driver);
>> -               if (err)
>> +               if (platform_driver_register(&iss_net_driver))
>>                          goto err_free_netdev;
>>                  driver_registered = 1;
>>          }
>> @@ -530,8 +527,7 @@ static int iss_net_configure(int index, char *init)
>>
>>          lp->pdev.id = index;
>>          lp->pdev.name = DRIVER_NAME;
>> -       err = platform_device_register(&lp->pdev);
>> -       if (err)
>> +       if (platform_device_register(&lp->pdev))
>>                  goto err_free_netdev;
>>          SET_NETDEV_DEV(dev, &lp->pdev.dev);
>>
>> @@ -541,23 +537,22 @@ static int iss_net_configure(int index, char *init)
>>          dev->irq = -1;
>>
>>          rtnl_lock();
>> -       err = register_netdevice(dev);
>> -       rtnl_unlock();
>> -
>> -       if (err) {
>> +       if (register_netdevice(dev)) {
>> +               rtnl_unlock();
>>                  pr_err("%s: error registering net device!\n", dev->name);
>>                  goto err_unregister_device;
>>          }
>> +       rtnl_unlock();
>>
>>          timer_setup(&lp->tl, iss_net_user_timer_expire, 0);
>>
>> -       return 0;
>> +       return;
>>
>>   err_unregister_device:
>>          platform_device_unregister(&lp->pdev);
>>   err_free_netdev:
>>          free_netdev(dev);
>> -       return err;
>> +       return;
> No need for 'return' at the end of the void function.
>