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

[Mike Frysinger <vapier.adi@gmail.com>]

On Thu, Mar 12, 2009 at 01:55, graff yang wrote:
> On Thu, Mar 12, 2009 at 12:59 PM, Mike Frysinger wrote:
>> On Thu, Mar 12, 2009 at 00:48, gyang wrote:
>>> On Thu, 2009-03-12 at 00:34 -0400, Mike Frysinger wrote:
>>>> On Thu, Mar 12, 2009 at 00:30, gyang wrote:
>>>> > 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.
>>>>
>>>> then the call to peripheral request would fail as expected
>>>
>>> For example, user want to enable irda on uart2 for bf548, he use id 0
>>> for uart2. In these case, peripheral request may not fail, but driver
>>> not work.
>>
>> the id is wrong then.  if they want uart2, then they should use id 2.
>
> Such wrong case may existed for common users, and should be
> prohibited. Driver need make sanity check to prevent it.

i dont know what you're talking about.  if the platform resource uses
id of 2, then they want uart2 which means irda2.  the sanity check is
already in place because the peripheral_request() will fail when it
attempts to claim the pins of uart2 but the uart driver has already
been enabled for it.
-mike