Re: [PATCH v2 1/3] media: rc: add support for Infrared Toy and IR Droid devices

[Oliver Neukum <oneukum@suse.com>]

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