On Sat, Feb 26, 2011 at 02:33:32PM +0100, Arvid Brodin wrote:
> Greg KH wrote:
> > On Fri, Feb 25, 2011 at 12:17:30AM +0100, Arvid Brodin wrote:
> >> This helps users with platform-bus-connected isp176xs with endianness problems
> >> and fixes a bug with non-32-bit io transfers on big endian hardware with
> >> straight data bus, where payload has to be byteswapped instead of ptds.
> >>
> >> Signed-off-by: Arvid Brodin <arvid.brodin@xxxxxxxx>
> >> ---
> >> drivers/usb/host/isp1760-hcd.c | 625 +++++++++++++++++++---------------------
> >> drivers/usb/host/isp1760-hcd.h | 33 +--
> >> 2 files changed, 315 insertions(+), 343 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
> >> index c470cc8..428a255 100644
> >> --- a/drivers/usb/host/isp1760-hcd.c
> >> +++ b/drivers/usb/host/isp1760-hcd.c
> >> @@ -114,73 +114,136 @@ struct isp1760_qh {
> >>
> >> #define ehci_port_speed(priv, portsc) USB_PORT_STAT_HIGH_SPEED
> >>
> >> -static unsigned int isp1760_readl(__u32 __iomem *regs)
> >> +static u32 isp176x_reg_read32(void __iomem *base, u32 reg)
> >> {
> >> - return readl(regs);
> >> + BUG_ON(reg >= 0x0400);
> >
> > Adding BUG() calls to drivers is not ok. You do NOT want to just freeze
> > a machine like this, that's unacceptable.
> >
> > Please redo this series and do not add any new calls that will crash a
> > box.
>
> I used those BUG() calls as an assert() while developing and though it might be
> a good idea to keep them for future developers, but sure, I can remove them
> with no ill effects.
>
> It's actually very wise not to allow these in new code; if the same rules had
> applied when this driver was first accepted, I would not have had to spend
> several hundred hours understanding and fixing this driver to remove the
> BUG() calls that are actually triggered in normal use... ;)
>
> Would it be correct to use WARN_ON() as a signal to future devs that they are
> not using the functions correctly? Or should I better just remove these checks?
WARN_ON() is fine for things that might go wrong, yes. But for
something as common as this call, you might just want to document it
very well, and leave it at that.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
