Re: [PATCH] [net/irda]: new Blackfin on-chip SIR IrDA driver

[gyang <graf.yang@analog.com>]

On Thu, 2009-03-12 at 00:23 -0400, Mike Frysinger wrote:
> On Thu, Mar 12, 2009 at 00:17, gyang wrote:
> > On Wed, 2009-03-11 at 06:43 -0400, Mike Frysinger wrote:
> >> On Wed, Mar 11, 2009 at 05:56, graff yang wrote:
> >> > On Wed, Mar 11, 2009 at 3:57 PM, Mike Frysinger wrote:
> >> >> On Wed, Mar 11, 2009 at 03:29,  <graff.yang@gmail.com> wrote:
> >> >>> +static int __devinit bfin_sir_probe(struct platform_device *pdev)
> >> >>> +{
> >> >>> +       struct net_device *dev;
> >> >>> +       struct bfin_sir_self *self;
> >> >>> +       unsigned int baudrate_mask;
> >> >>> +       struct bfin_sir_port *sir_port;
> >> >>> +       int err;
> >> >>> +
> >> >>> +       err = peripheral_request_list(per[pdev->id], DRIVER_NAME);
> >> >>
> >> >> what if pdev->id is set to 12512 ?
> >> >
> >> > The pdev->id is defined in board files, for example, uart0 should be 0.
> >>
> >> so ?  what's to stop the user from setting it to 12415 ?  the driver
> >> must sanity check these things.  you can never assume platform dev
> >> resources always have valid pointers and valid values.
> >
> > How about to limit the pdev->id to be 0,1,2,3 ?
> > I can extend the per to be
> > static const unsigned short per[][4] = {
> >        {P_UART0_RX, P_UART0_TX, 0, 0},
> >        {P_UART1_RX, P_UART1_TX, 0, 1},
> >        {P_UART2_RX, P_UART2_TX, 0, 2},
> >        {P_UART3_RX, P_UART3_TX, 0, 3},
> > };
> >
> > then check (pdev->id >= 0 && pdev->id < ARRAY_SIZE(per) &&
> > per[pdev->id][3] == pdev->id)
> 
> the last check is redundant.  the first two should be added.

I'm afraid of some cases such as uart0 uses id 1, will cause driver not
work, and not any warning message.

> 
> >> >>> +       switch (max_rate) {
> >> >>> +       default:
> >> >>> +               printk(KERN_WARNING "bfin_sir: Invalid maximum baud rate, using 9600\n");
> >> >>
> >> >> dev_warn(&dev->dev,....) ?
> >> >
> >> > Here the netdev has not been registered.
> >>
> >> then use pdev->dev ?
> >
> > Don't it seem strange to mixed use dev_warn(&dev->dev, ...) and
> > dev_warn(&pdev->dev, ...) ?
> 
> not really
> 
> > And printk is enough.
> 
> printk() makes sense when a dev is not available.  we have a dev here,
> so the dev_*() funcs should be used.
> -mike