[PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable

[PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable

[Andy Shevchenko]

Instead of putting garbage in the data structure, assign allocated id
or an error code to a temporary variable. This makes code cleaner.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/rpmsg_tty.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index 813076341ffd..8c17ddbf371d 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -121,15 +121,16 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
 		return ERR_PTR(-ENOMEM);
 
 	mutex_lock(&idr_lock);
-	cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
+	err = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
 	mutex_unlock(&idr_lock);
 
-	if (cport->id < 0) {
-		err = cport->id;
+	if (err < 0) {
 		kfree(cport);
 		return ERR_PTR(err);
 	}
 
+	cport->id = err;
+
 	return cport;
 }
 
-- 
2.33.0

[PATCH v1 2/5] tty: rpmsg: Unify variable used to keep an error code

[Andy Shevchenko]

In some ret is used, in the other err. Let's unify it across the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/rpmsg_tty.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index 8c17ddbf371d..fc5d33dc4cb8 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -114,22 +114,22 @@ static const struct tty_operations rpmsg_tty_ops = {
 static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
 {
 	struct rpmsg_tty_port *cport;
-	int err;
+	int ret;
 
 	cport = kzalloc(sizeof(*cport), GFP_KERNEL);
 	if (!cport)
 		return ERR_PTR(-ENOMEM);
 
 	mutex_lock(&idr_lock);
-	err = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
+	ret = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
 	mutex_unlock(&idr_lock);
 
-	if (err < 0) {
+	if (ret < 0) {
 		kfree(cport);
-		return ERR_PTR(err);
+		return ERR_PTR(ret);
 	}
 
-	cport->id = err;
+	cport->id = ret;
 
 	return cport;
 }
@@ -217,7 +217,7 @@ static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
 
 static int __init rpmsg_tty_init(void)
 {
-	int err;
+	int ret;
 
 	rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
 					    TTY_DRIVER_DYNAMIC_DEV);
@@ -236,15 +236,15 @@ static int __init rpmsg_tty_init(void)
 
 	tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
 
-	err = tty_register_driver(rpmsg_tty_driver);
-	if (err < 0) {
-		pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
+	ret = tty_register_driver(rpmsg_tty_driver);
+	if (ret < 0) {
+		pr_err("Couldn't install rpmsg tty driver: %d\n", ret);
 		goto error_put;
 	}
 
-	err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
-	if (err < 0) {
-		pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
+	ret = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
+	if (ret < 0) {
+		pr_err("Couldn't register rpmsg tty driver: %d\n", ret);
 		goto error_unregister;
 	}
 
@@ -256,7 +256,7 @@ static int __init rpmsg_tty_init(void)
 error_put:
 	tty_driver_kref_put(rpmsg_tty_driver);
 
-	return err;
+	return ret;
 }
 
 static void __exit rpmsg_tty_exit(void)
-- 
2.33.0

[PATCH v1 3/5] tty: rpmsg: Use dev_err_probe() in ->probe()

[Andy Shevchenko]

It's fine to use dev_err_probe() in ->probe() even if we know
it won't be deferred.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/rpmsg_tty.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index fc5d33dc4cb8..d172dd331bc9 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -153,10 +153,8 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
 	int ret;
 
 	cport = rpmsg_tty_alloc_cport();
-	if (IS_ERR(cport)) {
-		dev_err(dev, "Failed to alloc tty port\n");
-		return PTR_ERR(cport);
-	}
+	if (IS_ERR(cport))
+		return dev_err_probe(dev, PTR_ERR(cport), "Failed to alloc tty port\n");
 
 	tty_port_init(&cport->port);
 	cport->port.ops = &rpmsg_tty_port_ops;
@@ -164,9 +162,8 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
 	tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
 					   cport->id, dev);
 	if (IS_ERR(tty_dev)) {
-		dev_err(dev, "Failed to register tty port\n");
-		ret = PTR_ERR(tty_dev);
-		goto  err_destroy;
+		ret = dev_err_probe(dev, PTR_ERR(tty_dev), "Failed to register tty port\n");
+		goto err_destroy;
 	}
 
 	cport->rpdev = rpdev;
-- 
2.33.0

[PATCH v1 4/5] tty: rpmsg: Add pr_fmt() to prefix messages

[Andy Shevchenko]

Make all messages to be prefixed in a unified way.
Add pr_fmt() to achieve this.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/rpmsg_tty.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index d172dd331bc9..4bee6c5bbb53 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -10,6 +10,8 @@
  * The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented yet.
  */
 
+#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/rpmsg.h>
 #include <linux/slab.h>
@@ -235,13 +237,13 @@ static int __init rpmsg_tty_init(void)
 
 	ret = tty_register_driver(rpmsg_tty_driver);
 	if (ret < 0) {
-		pr_err("Couldn't install rpmsg tty driver: %d\n", ret);
+		pr_err("Couldn't install driver: %d\n", ret);
 		goto error_put;
 	}
 
 	ret = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
 	if (ret < 0) {
-		pr_err("Couldn't register rpmsg tty driver: %d\n", ret);
+		pr_err("Couldn't register driver: %d\n", ret);
 		goto error_unregister;
 	}
 
-- 
2.33.0

[PATCH v1 5/5] tty: rpmsg: Define tty name via constant string literal

[Andy Shevchenko]

Driver uses already twice the same string literal.

Define it in one place, so every user will have this
name consistent.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/rpmsg_tty.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
index 4bee6c5bbb53..dae2a4e44f38 100644
--- a/drivers/tty/rpmsg_tty.c
+++ b/drivers/tty/rpmsg_tty.c
@@ -18,6 +18,7 @@
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 
+#define RPMSG_TTY_NAME	"ttyRPMSG"
 #define MAX_TTY_RPMSG	32
 
 static DEFINE_IDR(tty_idr);	/* tty instance id */
@@ -172,7 +173,7 @@ static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
 
 	dev_set_drvdata(dev, cport);
 
-	dev_dbg(dev, "New channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
+	dev_dbg(dev, "New channel: 0x%x -> 0x%x: " RPMSG_TTY_NAME "%d\n",
 		rpdev->src, rpdev->dst, cport->id);
 
 	return 0;
@@ -224,7 +225,7 @@ static int __init rpmsg_tty_init(void)
 		return PTR_ERR(rpmsg_tty_driver);
 
 	rpmsg_tty_driver->driver_name = "rpmsg_tty";
-	rpmsg_tty_driver->name = "ttyRPMSG";
+	rpmsg_tty_driver->name = RPMSG_TTY_NAME;
 	rpmsg_tty_driver->major = 0;
 	rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
 
-- 
2.33.0

Re: [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable

[Arnaud POULIQUEN]

Hi Andy,

On 10/25/21 3:51 PM, Andy Shevchenko wrote:
> Instead of putting garbage in the data structure, assign allocated id
> or an error code to a temporary variable. This makes code cleaner.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/rpmsg_tty.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
> index 813076341ffd..8c17ddbf371d 100644
> --- a/drivers/tty/rpmsg_tty.c
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -121,15 +121,16 @@ static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
>  		return ERR_PTR(-ENOMEM);
>  
>  	mutex_lock(&idr_lock);
> -	cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
> +	err = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
>  	mutex_unlock(&idr_lock);
>  
> -	if (cport->id < 0) {
> -		err = cport->id;
> +	if (err < 0) {
>  		kfree(cport);
>  		return ERR_PTR(err);
>  	}
>  
> +	cport->id = err;

Set the cport->id to the err variable on success doesn't seem completely clean
to me either.
What about renaming "err" by "id" as done in [1]?

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/mps2-uart.c#L526

Regards,
Arnaud

> +
>  	return cport;
>  }
>  
> 

Re: [PATCH v1 1/5] tty: rpmsg: Assign returned id to a local variable

[Andy Shevchenko]

On Tue, Nov 2, 2021 at 3:21 PM Arnaud POULIQUEN
<arnaud.pouliquen@foss.st.com> wrote:
> On 10/25/21 3:51 PM, Andy Shevchenko wrote:

...

> > +     cport->id = err;
>
> Set the cport->id to the err variable on success doesn't seem completely clean
> to me either.

I totally agree with you, that's why the
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?id=408a507996e4
 changed it.

-- 
With Best Regards,
Andy Shevchenko