Re: [flashrom] [PATCH] Use extract_param everywhere

[Michael Karcher]

Am Montag, den 05.07.2010, 18:34 +0200 schrieb Carl-Daniel Hailfinger:
> > > int drkaiser_init(void)
> > >  {
> > >    uint32_t addr;
> > >
> > > char *devstring;
> > >
> > > get_io_perms();
> > >
> > > devstring = extract_param(&programmer_param, "pci", ",");
> > > pcidev_init(PCI_VENDOR_ID_DRKAISER, PCI_BASE_ADDRESS_2,
> > >     drkaiser_pcidev, programmer_param);
> > >     drkaiser_pcidev, devstring);
> > > free(devstring);
> >
> > can't we do the devsting stuff inside the pcidev_init function, if it is
> > the same for all programmers?
>
> Good point. Should I do that in a followup patch or in an update of this
> patch?

I suggest to do this in an update of this patch. This patch will
actually get smaller by it, as you don't introduce lots of duplicated
code.
> > > if (programmer_param && !strlen(programmer_param)) {
> > > 	free(programmer_param);
> > > 	programmer_param = NULL;
> > > /* Non-default port requested? */
> > > portpos = extract_param(&programmer_param, "it87spiport", ",:");
> > > if (portpos && strlen(portpos)) {
> > > 	flashport = strtol(portpos, (char **)NULL, 0);
> >
> > I know it wasn't in before, but some minimal error checking would be
> > really nice here. strtol returns 0 on failure, and we don't want to set
> > port 0. Think of "it87spiport=0y4800".
>
> True. Well, if my docs are correct, strtol("123foo456") will return 123.

Your docs are correct.
> If we really want error checking for the parameter, we probably need to
> check endptr in strtol, and should check if the value is nonzero and
> less than 65536 as well. Maybe even a check for alignment...

Right. This is the sophisticated and correct version. The classic test
is "*endptr == 0".
> The IT8705F support patch touches that area, and I'll extend it to take
> care of this issue as well.

OK.
> > > ret = buspirate_serialport_setup(dev);
> > > if (ret)
> > > return ret;
> > > free(dev);
> > > speed = extract_param(&programmer_param, "spispeed", ",:");
> > > if (speed) {
> > > for (i = 0; spispeeds[i].name; i++)
> > > 	if (!strncasecmp(spispeeds[i].name, speed,
> > >
> > > @@ -129,13 +123,11 @@
> > >    	if (!spispeeds[i].name)
> > >    		msg_perr("Invalid SPI speed, using default.\n");
> > >    }
> > > free(speed);
> > > /* This works because speeds numbering starts at 0 and is contiguous. */
> > >  msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed].name);
> > > ret = buspirate_serialport_setup(dev);
> > > if (ret)
> > > return ret;
> >
> > This hunk moves the initialization of the device. Is that intended?
>
> Yes. This allows us to abort init in case of broken parameters even
> before we start to touch the hardware. The biggest benefit is not having
> to shut down the serial port due to errors in parameters.

Uh, do I read the diff in a wrong way? It seems like you now move the
port setup function before parsing the speed while it was after parsing
the speed. So the new code does touch the hardware before noticing
broken parameters while the old code didn't.
> > > int programmer_init(void)
> > >  {
> > > return programmer_table[programmer].init();
> > > int ret;
> > > ret = programmer_table[programmer].init();
> > > if (programmer_param && strlen(programmer_param)) {
> >
> > In the end, this needs to be in a subfunction like
> > get_unhandled_parameters()
>
> Mh. You have a point. Given that the whole parameter handling stuff
> strips out the extracted parameters from programmer_param, the string
> should be empty in the end. Not sure if a single line checking for a
> non-empty programmer_param should be in its own function, though.

Sorry. I didn't get that this is the file programmer_param is declared
in. Just keep it this way.
> Thanks for the review. How should I proceed?

Please change the pcidev stuff to use common code in pcidev_init and
recheck the ordering in the buspirate file. With these changes:

Acked-by: Michael Karcher

Regards,
  Michael Karcher