diff mbox series

[v3,4.19.y-cip,12/17] spi: spi-mem: Compute length only when needed

Message ID 20210105135757.11069-13-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Accepted
Headers show
Series Add Renesas RPC-IF driver | expand

Commit Message

Lad Prabhakar Jan. 5, 2021, 1:57 p.m. UTC 
From: Tudor Ambarus <tudor.ambarus@microchip.com>

commit c0e035ac56680e74b27fc218c07e70f4b9b8b7a5 upstream.

When adjust_op_size is defined, len is never used. Move the len
computation where it's actually used.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Link: https://lore.kernel.org/r/20200228160735.1565047-1-tudor.ambarus@microchip.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/spi/spi-mem.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Pavel Machek Jan. 5, 2021, 4:53 p.m. UTC | #1 
Hi!

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> commit c0e035ac56680e74b27fc218c07e70f4b9b8b7a5 upstream.
> 
> When adjust_op_size is defined, len is never used. Move the len
> computation where it's actually used.

> index 59f7aa117a10..02cf8c3f7350 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -412,12 +412,13 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>  	struct spi_controller *ctlr = mem->spi->controller;
>  	size_t len;
>  
> -	len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
> -
>  	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
>  		return ctlr->mem_ops->adjust_op_size(mem, op);
>  
>  	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
> +		len = sizeof(op->cmd.opcode) + op->addr.nbytes +
> +		      op->dummy.nbytes;
> +
>  		if (len > spi_max_transfer_size(mem->spi))
>  			return -EINVAL;

If we are doing this kind of cleanups... I'd move "len" declaration
inside the if, too... Not that it matters much.

Best regards,

								Pavel

Signed-off-by: Pavel Machek (CIP) <pavel@denx.de>

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index c3a08afb6edf..bc64b4223913 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -410,13 +410,12 @@ EXPORT_SYMBOL_GPL(spi_mem_get_name);
 int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 {
 	struct spi_controller *ctlr = mem->spi->controller;
-	size_t len;
 
 	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
 		return ctlr->mem_ops->adjust_op_size(mem, op);
 
 	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
-		len = sizeof(op->cmd.opcode) + op->addr.nbytes +
+		size_t len = sizeof(op->cmd.opcode) + op->addr.nbytes +
 		      op->dummy.nbytes;
 
 		if (len > spi_max_transfer_size(mem->spi))
Lad Prabhakar Jan. 5, 2021, 5:10 p.m. UTC | #2 
Hi Pavel,

Thank you for the review.

> -----Original Message-----
> From: Pavel Machek <pavel@denx.de>
> Sent: 05 January 2021 16:54
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: cip-dev@lists.cip-project.org; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek
> <pavel@denx.de>; Biju Das <biju.das.jz@bp.renesas.com>
> Subject: Re: [PATCH v3 4.19.y-cip 12/17] spi: spi-mem: Compute length only when needed
> 
> Hi!
> 
> > From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >
> > commit c0e035ac56680e74b27fc218c07e70f4b9b8b7a5 upstream.
> >
> > When adjust_op_size is defined, len is never used. Move the len
> > computation where it's actually used.
> 
> > index 59f7aa117a10..02cf8c3f7350 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -412,12 +412,13 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> >  	struct spi_controller *ctlr = mem->spi->controller;
> >  	size_t len;
> >
> > -	len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
> > -
> >  	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
> >  		return ctlr->mem_ops->adjust_op_size(mem, op);
> >
> >  	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
> > +		len = sizeof(op->cmd.opcode) + op->addr.nbytes +
> > +		      op->dummy.nbytes;
> > +
> >  		if (len > spi_max_transfer_size(mem->spi))
> >  			return -EINVAL;
> 
> If we are doing this kind of cleanups... I'd move "len" declaration
> inside the if, too... Not that it matters much.
> 
Should live without this change, will drop this for now.

Cheers,
Prabhakar

> Best regards,
> 
> 								Pavel
> 
> Signed-off-by: Pavel Machek (CIP) <pavel@denx.de>
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index c3a08afb6edf..bc64b4223913 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -410,13 +410,12 @@ EXPORT_SYMBOL_GPL(spi_mem_get_name);
>  int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>  {
>  	struct spi_controller *ctlr = mem->spi->controller;
> -	size_t len;
> 
>  	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
>  		return ctlr->mem_ops->adjust_op_size(mem, op);
> 
>  	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
> -		len = sizeof(op->cmd.opcode) + op->addr.nbytes +
> +		size_t len = sizeof(op->cmd.opcode) + op->addr.nbytes +
>  		      op->dummy.nbytes;
> 
>  		if (len > spi_max_transfer_size(mem->spi))
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#6038): https://lists.cip-project.org/g/cip-dev/message/6038
Mute This Topic: https://lists.cip-project.org/mt/79450076/4520388
Group Owner: cip-dev+owner@lists.cip-project.org
Unsubscribe: https://lists.cip-project.org/g/cip-dev/leave/8129055/727948398/xyzzy [cip-dev@archiver.kernel.org]
-=-=-=-=-=-=-=-=-=-=-=-
Pavel Machek Jan. 5, 2021, 5:29 p.m. UTC | #3 
Hi!

> > > commit c0e035ac56680e74b27fc218c07e70f4b9b8b7a5 upstream.
> > >
> > > When adjust_op_size is defined, len is never used. Move the len
> > > computation where it's actually used.
> > 
> > > index 59f7aa117a10..02cf8c3f7350 100644
> > > --- a/drivers/spi/spi-mem.c
> > > +++ b/drivers/spi/spi-mem.c
> > > @@ -412,12 +412,13 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> > >  	struct spi_controller *ctlr = mem->spi->controller;
> > >  	size_t len;
> > >
> > > -	len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
> > > -
> > >  	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
> > >  		return ctlr->mem_ops->adjust_op_size(mem, op);
> > >
> > >  	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
> > > +		len = sizeof(op->cmd.opcode) + op->addr.nbytes +
> > > +		      op->dummy.nbytes;
> > > +
> > >  		if (len > spi_max_transfer_size(mem->spi))
> > >  			return -EINVAL;
> > 
> > If we are doing this kind of cleanups... I'd move "len" declaration
> > inside the if, too... Not that it matters much.
> > 
> Should live without this change, will drop this for now.

We can live with or without this one, I'd say.

You do not need to do anything at the moment: I believe we can simply
apply the series as-is.

Best regards,
     								Pavel
diff mbox series

Patch

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 59f7aa117a10..02cf8c3f7350 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -412,12 +412,13 @@  int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 	struct spi_controller *ctlr = mem->spi->controller;
 	size_t len;
 
-	len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
-
 	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
 		return ctlr->mem_ops->adjust_op_size(mem, op);
 
 	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
+		len = sizeof(op->cmd.opcode) + op->addr.nbytes +
+		      op->dummy.nbytes;
+
 		if (len > spi_max_transfer_size(mem->spi))
 			return -EINVAL;