Am Mittwoch, den 27.05.2020, 13:28 +0100 schrieb Sean Young:
Hi,
> > This violates the DMA coherency rules. The buffers must be
> > allocated separately with kmalloc().
>
> Right, I'll fix this and send out a v3. There are other usb drivers in
> drivers/media/rc/.. that break this rule too.
Unfortunately.
> > > + case STATE_IRDATA: {
> > > + struct ir_raw_event rawir = { .pulse = irtoy->pulse };
> > > + __be16 *in = (__be16 *)irtoy->in;
> > > + int i;
> > > +
> > > + for (i = 0; i < len / sizeof(__be16); i++) {
> > > + u32 v = be16_to_cpup(in + i);
> >
> > Is this 16 or 32 bit?
>
> It's 16 bit but I would like it up-cast so that v * UNIT_NS is a 32 bit
> multiply. This could do with a comment. Also could be be16_to_cpu(in[i]).
That is nasty. I'd say if you need a larger size, cast explicitly.
> > > +static int irtoy_suspend(struct usb_interface *intf, pm_message_t message)
> > > +{
> > > + struct irtoy *irtoy = usb_get_intfdata(intf);
> > > +
> > > + usb_kill_urb(irtoy->urb_in);
> > > + usb_kill_urb(irtoy->urb_out);
> >
> > That is brutal. It could fail commands. Do you really want to
> > do that?
>
> Commands can only be sent during 1) device probe and 2) ir transmit. During
> ir transmit we are non-interruptable process context, so we should not end up
> here unless I'm mistaken.
Well. then the kill is redundant. The freezer will save you if the
system sleeps. If somebody switches on runtime PM. you are potentially
in trouble.
> When we're not issuing commands we're waiting for ir receive; should that
> urb be killed for the duration of suspend?
No IO to a suspended device.
Regards
Oliver
