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.
> 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).
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.
Perhaps Greg (or someone else) want to comment on this? I'm happy either way,
except it's easier for me not to modify the patch. ;)
>> diff --git a/drivers/usb/host/isp1760-hcd.c
>> b/drivers/usb/host/isp1760-hcd.c
>> index 3a6743d..eca975b 100644
>> --- a/drivers/usb/host/isp1760-hcd.c
>> +++ b/drivers/usb/host/isp1760-hcd.c
>> @@ -379,7 +379,7 @@ static int ehci_reset(struct usb_hcd *hcd)
>>
>> static void qh_destroy(struct isp1760_qh *qh)
>> {
>> - BUG_ON(!list_empty(&qh->qtd_list));
>> + WARN_ON(!list_empty(&qh->qtd_list));
>> kmem_cache_free(qh_cachep, qh);
>
> You free a qh which has still qtd queued. This means that you have a qh or
> qtd pointer stored somewhere. So you don't bug here but later somewhere
> once that URB completes (unless it is lost).
>
>
>> @@ -1605,7 +1536,7 @@ static int isp1760_urb_dequeue(struct usb_hcd
>> *hcd, struct urb *urb, int status)
>> for (i = 0; i < 32; i++) {
>> if (!ints[i].qh)
>> continue;
>> - BUG_ON(!ints[i].qtd);
>> + WARN_ON(!ints[i].qtd);
>>
>> if (ints[i].qtd->urb == urb) {
>> u32 skip_map;
>
> Instead of a BUG if there is a NULL pointer where we expect something
> valid we get a backtrace (which is also provided by BUG_ON) and we
> dereference the NULL pointer.
>
> Sebastian
--
Arvid Brodin
Enea Services Stockholm AB
--
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
