On Sun, 2014-09-14 at 11:26 +0200, Hans de Goede wrote:
> Hi,
>
> On 09/13/2014 09:31 PM, Sergei Shtylyov wrote:
> > Hello.
> >
> > On 9/13/2014 1:26 PM, Hans de Goede wrote:
> >
> >> The data urbs are all killed before calling zap_pending, and their completion
> >> handler should have cleared their inflight flag.
> >
> >> Do not 0 the data inflight flags, and add a check for try_complete succeeding,
> >> as it should always succeed when called from zap_pending.
> >
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> drivers/usb/storage/uas.c | 10 +++++-----
> >> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> >> index 08edb6b..85bbc1d 100644
> >> --- a/drivers/usb/storage/uas.c
> >> +++ b/drivers/usb/storage/uas.c
> >> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
> >> struct uas_cmd_info *cmdinfo;
> >> struct uas_cmd_info *temp;
> >> unsigned long flags;
> >> + int err;
> >
> > Er, I don't see why this variable is necessary.
> >
> > [...]
> >> @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result)
> >> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd,
> >> SCp);
> >> uas_log_cmd_state(cmnd, __func__);
> >> - /* all urbs are killed, clear inflight bits */
> >> - cmdinfo->state &= ~(COMMAND_INFLIGHT |
> >> - DATA_IN_URB_INFLIGHT |
> >> - DATA_OUT_URB_INFLIGHT);
> >> + /* Sense urbs were killed, clear COMMAND_INFLIGHT manually */
> >> + cmdinfo->state &= ~COMMAND_INFLIGHT;
> >> cmnd->result = result << 16;
> >> - uas_try_complete(cmnd, __func__);
> >> + err = uas_try_complete(cmnd, __func__);
> >> + WARN_ON(err != 0);
> >
> > Why not:
> >
> > WARN_ON(uas_try_complete(cmnd, __func__));
> >
>
> This was discussed already during v1 of this patch-set, WARN_ON may
> not have a side-effect, as it may be defined as an empty macro.
Must have missed the discussion, but whoever said that loses all their
review points. We're very careful to make sure that even in the case
where WARN_ON and BUG_ON (and indeed any macros) are compiled out, the
side effects are still accounted for. This is the canonical definition
of WARN_ON in the compiled out case:
#define WARN_ON(condition) ({ \
int __ret_warn_on = !!(condition); \
unlikely(__ret_warn_on); \
})
So the compiler will eliminate the statement only if there are no side
effects.
James
--
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
