[U-Boot,v3,082/108] spi: ich: Correct max-size bug in ich_spi_adjust_size()
diff mbox series

Message ID 20191021033913.220758-77-sjg@chromium.org
State Superseded
Delegated to: Bin Meng
Headers show
Series
  • x86: Add initial support for apollolake
Related show

Commit Message

Simon Glass Oct. 21, 2019, 3:38 a.m. UTC
This incorrectly shortens read operations if there is a maximum write size
but no maximum read size. Fix it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v3: None
Changes in v2: None

 drivers/spi/ich.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Bin Meng Nov. 19, 2019, 2:36 p.m. UTC | #1
Hi Simon,

On Mon, Oct 21, 2019 at 11:40 AM Simon Glass <sjg@chromium.org> wrote:
>
> This incorrectly shortens read operations if there is a maximum write size
> but no maximum read size. Fix it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/spi/ich.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index 7f73f096ecb..b28f3423584 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -424,9 +424,11 @@ static int ich_spi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
>                 page_offset = do_div(aux, ICH_BOUNDARY);
>         }
>
> -       if (op->data.dir == SPI_MEM_DATA_IN && slave->max_read_size) {
> -               op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> -                                     slave->max_read_size);
> +       if (op->data.dir == SPI_MEM_DATA_IN) {
> +               if (slave->max_read_size) {
> +                       op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> +                                             slave->max_read_size);
> +               }

Isn't the changed one the same as the old codes?

>         } else if (slave->max_write_size) {
>                 op->data.nbytes = min(ICH_BOUNDARY - page_offset,
>                                       slave->max_write_size);
> --

Regards,
Bin
Simon Glass Nov. 21, 2019, 1:50 p.m. UTC | #2
Hi Bin,

On Tue, 19 Nov 2019 at 07:36, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Oct 21, 2019 at 11:40 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > This incorrectly shortens read operations if there is a maximum write size
> > but no maximum read size. Fix it.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  drivers/spi/ich.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> > index 7f73f096ecb..b28f3423584 100644
> > --- a/drivers/spi/ich.c
> > +++ b/drivers/spi/ich.c
> > @@ -424,9 +424,11 @@ static int ich_spi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
> >                 page_offset = do_div(aux, ICH_BOUNDARY);
> >         }
> >
> > -       if (op->data.dir == SPI_MEM_DATA_IN && slave->max_read_size) {
> > -               op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> > -                                     slave->max_read_size);
> > +       if (op->data.dir == SPI_MEM_DATA_IN) {
> > +               if (slave->max_read_size) {
> > +                       op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> > +                                             slave->max_read_size);
> > +               }
>
> Isn't the changed one the same as the old codes?

The old code fails if max_read_size is 0 but max_write_size is set. It
forgets to take account of max_write_size in that case.


>
> >         } else if (slave->max_write_size) {
> >                 op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> >                                       slave->max_write_size);
> > --

Regards,
Simon
Bin Meng Nov. 21, 2019, 2:04 p.m. UTC | #3
Hi Simon,

On Thu, Nov 21, 2019 at 9:50 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Tue, 19 Nov 2019 at 07:36, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Oct 21, 2019 at 11:40 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > This incorrectly shortens read operations if there is a maximum write size
> > > but no maximum read size. Fix it.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > >  drivers/spi/ich.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> > > index 7f73f096ecb..b28f3423584 100644
> > > --- a/drivers/spi/ich.c
> > > +++ b/drivers/spi/ich.c
> > > @@ -424,9 +424,11 @@ static int ich_spi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
> > >                 page_offset = do_div(aux, ICH_BOUNDARY);
> > >         }
> > >
> > > -       if (op->data.dir == SPI_MEM_DATA_IN && slave->max_read_size) {
> > > -               op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> > > -                                     slave->max_read_size);
> > > +       if (op->data.dir == SPI_MEM_DATA_IN) {
> > > +               if (slave->max_read_size) {
> > > +                       op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> > > +                                             slave->max_read_size);
> > > +               }
> >
> > Isn't the changed one the same as the old codes?
>
> The old code fails if max_read_size is 0 but max_write_size is set. It
> forgets to take account of max_write_size in that case.
>

I still don't get this. Based on your description, it seems that your
logic works if we remove the } before the else if

>
> >
> > >         } else if (slave->max_write_size) {
> > >                 op->data.nbytes = min(ICH_BOUNDARY - page_offset,
> > >                                       slave->max_write_size);
> > > --
>

Regards,
Bin

Patch
diff mbox series

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index 7f73f096ecb..b28f3423584 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -424,9 +424,11 @@  static int ich_spi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
 		page_offset = do_div(aux, ICH_BOUNDARY);
 	}
 
-	if (op->data.dir == SPI_MEM_DATA_IN && slave->max_read_size) {
-		op->data.nbytes = min(ICH_BOUNDARY - page_offset,
-				      slave->max_read_size);
+	if (op->data.dir == SPI_MEM_DATA_IN) {
+		if (slave->max_read_size) {
+			op->data.nbytes = min(ICH_BOUNDARY - page_offset,
+					      slave->max_read_size);
+		}
 	} else if (slave->max_write_size) {
 		op->data.nbytes = min(ICH_BOUNDARY - page_offset,
 				      slave->max_write_size);