Re: [PATCH] tools/mrvl_uart.sh: Remove script

["Pali Rohár" <pali@kernel.org>]

On Monday 07 February 2022 08:20:54 Marcel Ziswiler wrote:
> On Sat, 2022-02-05 at 15:54 +0100, Pali Rohár wrote:
> > On Saturday 05 February 2022 03:07:00 Marcel Ziswiler wrote:
> > > On Sat, 2022-02-05 at 01:54 +0100, Pali Rohár wrote:
> > > > On Saturday 05 February 2022 01:40:23 Marcel Ziswiler wrote:
> > > > > On Sat, 2022-02-05 at 01:25 +0100, Pali Rohár wrote:
> > > > > > $ kwboot -b /dev/ttyUSB0
> > > > > 
> > > > > Hm, at least kwboot from today's master does not allow -b without also
> > > > > giving it an image.
> > > > 
> > > > This commit is part of master branch and added support for it:
> > > > https://source.denx.de/u-boot/u-boot/-/commit/c513fe47dca24de87a904ce7d71cfd8a390e1154
> > > > 
> > > > If it does not work then there is some bug which should be fixed. I have
> > > > tested it and it works on my setup.
> > > > 
> > > > Can you check if you have that commit to ensure that we are not going to
> > > > test / debug older version?
> > > 
> > > Yes, sure. I debugged it a little and I believe I found the issue. I guess the intention was for it to be
> > > run
> > > giving it a dash '-' as image argument for skipping. (Even though that usually would mean using stdin).
> > > Anyway,
> > > it fails as it then uses such dash as ttypath because optind did not get incremented. The following fixes
> > > it
> > > for me:
> > > 
> > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > index 2684f0e75a..d490c026e9 100644
> > > --- a/tools/kwboot.c
> > > +++ b/tools/kwboot.c
> > > @@ -1768,7 +1768,8 @@ main(int argc, char **argv)
> > >                         if (prev_optind == optind)
> > >                                 goto usage;
> > >                         if (argv[optind] && argv[optind][0] != '-')
> > > -                               imgpath = argv[optind++];
> > > +                               imgpath = argv[optind];
> > > +                       optind++;
> > >                         break;
> > >  
> > >                 case 'D':
> > > 
> > > Let me know if that is indeed the intention and I can send a proper fix for it. Maybe I can also update the
> > > documentation/usage in that respect. Thanks again.
> > 
> > Now I see where is the issue. Your fix is incorrect because it breaks
> > other usage (e.g. specifying other options).
> > 
> > The issue here is that argv[optind] is used also when it is the last
> > option on the command line -- which is not getopt() at all, as it is
> > parsed after the getopt() processing.
> > 
> > So I think that correct fix should be something like this:
> > 
> >                         bootmsg = kwboot_msg_boot;
> >                         if (prev_optind == optind)
> >                                 goto usage;
> > -                       if (argv[optind] && argv[optind][0] != '-')
> > +                       if (optind < argc-1 && argv[optind] && argv[optind][0] != '-')
> >                                 imgpath = argv[optind++];
> >                         break;
> >  
> > 
> > This should fix usage of all "kwboot -b /dev/tty", "kwboot -b -t /dev/tty"
> > and "kwboot -b image.kwb /dev/tty" usage.
> > 
> > Could you test it?
> 
> Yes, this indeed works fine now.
> 
> With that, and if we properly document such "standalone" usage, I would be fine with dropping
> tools/mrvl_uart.sh. So you can get my reviewed-by for this one.
> 
> Reviewed-by: Marcel Ziswiler <marcel@ziswiler.com>
> 
> And you get my tested-by for such patch fixing this as outlined above.
> 
> Tested-by: Marcel Ziswiler <marcel@ziswiler.com>
> 
> > Seems that I tested only -b -t combination (as I wanted to see that
> > bootrom correctly start sending NAK xmodem bytes).
> 
> Yep, now I got you. Thanks!

Ok, thank you for testing, I will send a proper patch to ML.