[U-Boot] [PATCH v5 071/101] spi: ich: Correct max-size bug in ich_spi_adjust_size()

[Simon Glass]

Hi Bin,

On Wed, 27 Nov 2019 at 00:16, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Nov 25, 2019 at 12:12 PM Simon Glass <sjg at 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 at chromium.org>
> > ---
> >
> > Changes in v5: None
> > Changes in v4: None
> > 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 08c37ca4ab..17b7a0ba0b 100644
> > --- a/drivers/spi/ich.c
> > +++ b/drivers/spi/ich.c
> > @@ -425,9 +425,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);
> > +               }
>
> I wrote the following comments in v3, but looks did not get clarified?
>
> 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);
> > --

You can try out a dry run...

Say:
max_read_size = 0
max_write_size = 64

Say we are doing a read...

> if (op->data.dir == SPI_MEM_DATA_IN && slave->max_read_size) {

This branch will not be taken. data.dir is ...IN but because the
max_read_size is 0, we skip this bit.

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

This branch is taken. Even though we are not doing a write, we set nbytes to 64:

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

So now we are doing a read but using the max_write_size. This is incorrect.

So we need to separate these two things, since otherwise reads get
limited when they should not be.

Regards,
Simon