Re: [PATCH] PCI serial card support

[n0ano]

On Thu, Nov 13, 2008 at 08:05:37PM +0200, Vesa J??skel?inen wrote:
> 
> Why not hide the legacy comports if they are not there and we can auto
> detect that?

I went back and forth on this.  I finally decided that COM1 to COM4 are
so firmly a part of the PC design I should leave them (note that if
GRUB_MACHINE_PCBIOS is not defined then these 4 I/O ports will be defined
whether or not they are there and I'll have to list them.).  If there's
a strong preference I can drop them if the BIOS doesn't define an I/O
port for them.

> 
> That actually brings us to other issue. Should be have some kind of HW
> dependant device path support that would be universal in grub?. Now if
> user unplugs in example USB serial converter then the order will change
> and can cause some problems.

Maybe.  Seems like a little bit of over engineering right now.

> 
> > --- include/grub/pci_serial_ids.h   (revision 0)
> > +++ include/grub/pci_serial_ids.h   (revision 0)
> > @@ -0,0 +1,642 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2000,2001,2002,2003,2004,2005,2007  Free Software 
> > Foundation, Inc.
> 
> This seems to be new file. Please change copyright years to start from 2008.
> 
> Where did you get that device table?. Generated by yourself from pci
> database?
> 
> Anyway. I think those vendor and device ID's should be in global file as
> they are "universal" to any PCI device.

It is now.  My understanding is that a data table is not copyrightable
(no expressive content) but, just to make sure there is no issue, I've
replaced this with a table of just the devices I know about.  I'll
create (a very small) generic PCI ID table, we can add on as time goes
by.

> 
> > --- term/i386/pc/serial.c   (revision 1911)
> > +++ term/i386/pc/serial.c   (working copy)
> 
> > +#include <grub/pci.h>
> >  
> > +void grub_serial_add(int type, unsigned int id, unsigned int base, 
> > unsigned int port);
> > +#include <grub/pci_serial_ids.h>
> > +
> 
> Please keep includes at same place.
> 
> Why is this prototype even here?. And should it be static?

Since the mapping table code calls this function the prototype
needs to be defined before the include of `pci_serial_ids.h'.
I'll move the prototype to `serial.h' and clean these two up.

> 
> > +char *serial_types[] = {
> > +   "legacy",
> > +   "   pci",
> > +   "   usb"
> > +};
> 
> static const
> 
> And please leave output formatting to actual printing.

sure.

> 
> > +  grub_printf("%c%2d: %s ", (p == serial_dev) ? '*' : ' ',
> > +                      i, serial_types[p->type]);
> 
> What is p->type is not within range of serial_types?

Then we have a more serious problem since `p->type' is only set
by the code to a value between 0 - 2.  Given that this could only
be wrong because of a code bug it seems silly to value check it.

> 
> > +char parity[] = {
> 
> static const
> 
> > +static int
> > +serial_pci_scan (int bus, int dev, int func, grub_pci_id_t pciid)
> 
> > +  vid = pciid & 0xffff;
> > +  did = pciid >> 16;
> > +  addr = grub_pci_make_address (bus, dev, func, 2);
> > +  w = grub_pci_read (addr);
> > +  class = (w >> 16) | ((w >> 8) & 0xff);
> > +  addr = grub_pci_make_address (bus, dev, func, 11);
> > +  w = grub_pci_read (addr);
> > +  ss_vid = w & 0xffff;
> > +  ss_did = w >> 16;
> 
> This smells. Perhaps helper function or macro.
> 
> As a generic note. If this is generic information that is available on
> every PCI device. Why not change PCI to iterate with structure.
> 
> Well... I didn't look too deep there this time. :)

I was trying not to re-write the PCI code too much, it is way to
hard coded right now but my goal was to get the serial support in
first.  I can certainly look into regularizing the PCI code a little
on the way.

Thanks for the comments, I'll get an updated patch out soon.


-- 
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
address@hidden
Ph: 303/443-3786