* Arvid Brodin | 2011-04-27 22:29:39 [+0200]:
>Sebastian Andrzej Siewior wrote:
>> Arvid Brodin wrote:
>>
>> The review would be easier if you would move the functions. Now diff shows
>> the removal of one function and the addition of another one.
>
>Actually, I think the problems are due to the renaming of three functions:
>
>isp1760_qtd_alloc() -> qtd_alloc()
>isp1760_qtd_free() -> qtd_free()
>qh_urb_transaction() -> packetize_urb()
>
>I also moved qtd_alloc() so that it is declared just before qtd_free() for
>clarity (well, at least that grouping is clearer to me).
>
>I realise it would have been better to separate the renames and the move to
>a separate patch. If the explanation above does not help, then please tell
>me and I will split the patch.
If you could make one patch which renames and re-groups the code (no
code change) and another patch which does the code changes then it would
be easier during a bisect run later on in case something brokes.
>> I don't like the replacement of BUG_ON() with WARN_ON() without changing
>> code. Here an example:
>
>I'm inclined to agree with you. However, this is Greg K-H's comments on a
>previous patch of mine:
>
>Greg Kroah-Hartman wrote:
>>> -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.
>>
>> thanks,
>>
>> greg k-h
>
>This was a similar situation to the ones you comment on below (i.e. something
>very fishy is going on: we're writing to an address that may not even belong
>to us).
You check the address if it is valid and BUG if it is not. What Greg
probably meant was to print something and return doing nothing. So you
don't freeze the box. In this case you should never pass reg >0x400 and
this static code and easy to audit.
>So the idea here was to make the driver more in line with kernel standards
>(or at least Greg's idea of them ;). Anyway, in the cases below I guess it won't
>make much of a difference whether we use BUG_ON() or WARN_ON()? We get the
>backtrace and (probably) a kernel panic in both cases.
There is little to none difference between the derefence of a NULL
pointer and the BUG() statement. You could try recover from the "invalid
state" by skipping the null pointer and doing nothing.
Could you please repost the patch series without the mass conversion
BUG->WARN?
Sebastian
--
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
