[U-Boot] [PATCH] sata: fix sata command not being executed bug

[Simon Glass]

Hi,

On 15 November 2016 at 00:20, Y.T. Tang <yuantian.tang at nxp.com> wrote:
> Hi Simon,
>
> Please see my reply inline.
>
>> -----Original Message-----
>> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
>> Sent: Saturday, November 12, 2016 12:18 AM
>> To: Y.T. Tang <yuantian.tang at nxp.com>
>> Cc: Bin Meng <bmeng.cn at gmail.com>; U-Boot Mailing List <u-
>> boot at lists.denx.de>
>> Subject: Re: [PATCH] sata: fix sata command not being executed bug
>>
>> Hi Tang,
>>
>> On 9 November 2016 at 02:37,  <yuantian.tang at nxp.com> wrote:
>> > From: Tang Yuantian <Yuantian.Tang at nxp.com>
>> >
>> > Variable sata_curr_device is used to indicate if there is a available
>> > sata disk on board.
>> >
>> > Previously, sata_curr_device is set in sata_initialize().
>> > Now, sata_initialize() is separated from other sata commands.
>> > Accordingly, sata_curr_device is removed from sata_initialize() too.
>> > This caused sata_curr_device never gets a chance to be set.
>> > If it can't be set a proper value, other sata command will never get a
>> > change to execute.
>> >
>> > This patch sets variable sata_curr_device properly.
>> >
>> > Signed-off-by: Tang Yuantian <yuantian.tang at nxp.com>
>> > ---
>> >  cmd/sata.c | 13 ++++++++++---
>> >  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> Does this fix commit d97dc8a0 - if so can you please add a 'Fixes:' tag?
>>
> Yes, commit d97dc8a0 causes this issue.
> There was a 'fix' in title. Do you mean adding "Fixes commit d97dc8a0"?

See the commit log for examples - here's one I found:

   Fixes: e824cf3f (smbios: Allow compilation on 64bit systems)

>
>> >
>> > diff --git a/cmd/sata.c b/cmd/sata.c
>> > index d18b523..71c785f 100644
>> > --- a/cmd/sata.c
>> > +++ b/cmd/sata.c
>> > @@ -20,6 +20,7 @@ static int sata_curr_device = -1;  static int
>> > do_sata(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])  {
>> >         int rc = 0;
>> > +       int i;
>> >
>> >         if (argc == 2 && strcmp(argv[1], "stop") == 0)
>> >                 return sata_stop();
>> > @@ -32,9 +33,15 @@ static int do_sata(cmd_tbl_t *cmdtp, int flag, int argc,
>> char * const argv[])
>> >         }
>> >
>> >         /* If the user has not yet run `sata init`, do it now */
>> > -       if (sata_curr_device == -1)
>> > -               if (sata_initialize())
>> > -                       return 1;
>> > +       if (sata_curr_device == -1) {
>> > +               rc = sata_initialize();
>> > +               for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++) {
>> > +                       if (sata_dev_desc[i].lba > 0)
>> > +                               sata_curr_device = i;
>> > +               }
>> > +               if (sata_curr_device == -1)
>> > +                       return -1;
>>
>> Can this code go in its own function?
>>
> I want to refine this patch to make it more clear. This part will be rewritten.

Regards,
Simon