Re: [RFC][PATCH 1/4 v3] usb: host: ehci-platform: BUG_ON() to WARN_ON() on probe

[Alan Stern]

On Mon, 6 Aug 2012, Simon Horman wrote:

> On Sun, Aug 05, 2012 at 06:51:34PM -0700, kuninori.morimoto...@renesas.com 
> wrote:
> > We should avoid using BUG_ON() in drivers.
> > This patch switch to use WARN_ON() from BUG_ON(),
> > and avoid NULL pointer access by new macro.
> > 
> > Signed-off-by: Kuninori Morimoto 
> > ---
> > v2 -> v3
> > 
> >  - BUG_ON -> WARN_ON
> > 
> >  drivers/usb/host/ehci-platform.c |   18 ++
> >  1 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-platform.c 
> > b/drivers/usb/host/ehci-platform.c
> > index 4b1d896..db27dfe 100644
> > --- a/drivers/usb/host/ehci-platform.c
> > +++ b/drivers/usb/host/ehci-platform.c
> > @@ -21,6 +21,8 @@
> >  #include 
> >  #include 
> >  
> > +#define ehci_pdata_get(pdata, x) ((pdata) ? (pdata)->x : 0)
> > +
> 
> FWIW, I think that an inline function would be a slight improvement over
> a macro here.  Likewise for the 2nd patch of this series.

No; we should not have either an inline function or a macro.  This 
computation isn't necessary at all.

If you're worried about the possibility that pdata might be NULL, 
just return -ENODEV from ehci_platform_probe() if it is:

int irq;
int err = -ENOMEM;

-   BUG_ON(!dev->dev.platform_data);
+   if (!dev->dev.platform_data) {
+   WARN_ON(1);
+   return -ENODEV;
+   }

if (usb_disabled())
return -ENODEV;

But even that may be unnecessary.  Simply removing the BUG_ON will
accomplish pretty much the same thing; people will realize something is
wrong pretty quickly when the computer gets an access violation from
trying to dereference a null pointer.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH 1/4 v3] usb: host: ehci-platform: BUG_ON() to WARN_ON() on probe

[Kuninori Morimoto]

Hi Simon

Thank you for checking patch

> > +#define ehci_pdata_get(pdata, x) ((pdata) ? (pdata)->x : 0)
> > +
> 
> FWIW, I think that an inline function would be a slight improvement over
> a macro here.  Likewise for the 2nd patch of this series.

All these patches have this kind of macros.
I needs maintainer's opinion.
If he said it should be inline, I'm happy to send v4 patches.

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH 1/4 v3] usb: host: ehci-platform: BUG_ON() to WARN_ON() on probe

[Simon Horman]

On Sun, Aug 05, 2012 at 06:51:34PM -0700, kuninori.morimoto...@renesas.com 
wrote:
> We should avoid using BUG_ON() in drivers.
> This patch switch to use WARN_ON() from BUG_ON(),
> and avoid NULL pointer access by new macro.
> 
> Signed-off-by: Kuninori Morimoto 
> ---
> v2 -> v3
> 
>  - BUG_ON -> WARN_ON
> 
>  drivers/usb/host/ehci-platform.c |   18 ++
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-platform.c 
> b/drivers/usb/host/ehci-platform.c
> index 4b1d896..db27dfe 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
>  
> +#define ehci_pdata_get(pdata, x) ((pdata) ? (pdata)->x : 0)
> +

FWIW, I think that an inline function would be a slight improvement over
a macro here.  Likewise for the 2nd patch of this series.

>  static int ehci_platform_reset(struct usb_hcd *hcd)
>  {
>   struct platform_device *pdev = to_platform_device(hcd->self.controller);
> @@ -28,19 +30,19 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
>   struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>   int retval;
>  
> - hcd->has_tt = pdata->has_tt;
> - ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
> - ehci->big_endian_desc = pdata->big_endian_desc;
> - ehci->big_endian_mmio = pdata->big_endian_mmio;
> + hcd->has_tt = ehci_pdata_get(pdata, has_tt);
> + ehci->has_synopsys_hc_bug = ehci_pdata_get(pdata, has_synopsys_hc_bug);
> + ehci->big_endian_desc = ehci_pdata_get(pdata, big_endian_desc);
> + ehci->big_endian_mmio = ehci_pdata_get(pdata, big_endian_mmio);
>  
> - ehci->caps = hcd->regs + pdata->caps_offset;
> + ehci->caps = hcd->regs + ehci_pdata_get(pdata, caps_offset);
>   retval = ehci_setup(hcd);
>   if (retval)
>   return retval;
>  
> - if (pdata->port_power_on)
> + if (ehci_pdata_get(pdata, port_power_on))
>   ehci_port_power(ehci, 1);
> - if (pdata->port_power_off)
> + if (ehci_pdata_get(pdata, port_power_off))
>   ehci_port_power(ehci, 0);
>  
>   return 0;
> @@ -85,7 +87,7 @@ static int __devinit ehci_platform_probe(struct 
> platform_device *dev)
>   int irq;
>   int err = -ENOMEM;
>  
> - BUG_ON(!dev->dev.platform_data);
> + WARN_ON(!dev->dev.platform_data);
>  
>   if (usb_disabled())
>   return -ENODEV;
> -- 
> 1.7.5.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html