Re: [PATCHv2 28/29] mxl111sf: Don't use dynamic static allocation

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

Em Sun, 3 Nov 2013 19:50:02 -0500
Michael Krufky <mkrufky@kernellabs.com> escreveu:

> On Sat, Nov 2, 2013 at 9:31 AM, Mauro Carvalho Chehab
> <m.chehab@samsung.com> wrote:
> > Dynamic static allocation is evil, as Kernel stack is too low, and
> > compilation complains about it on some archs:
> >
> >         drivers/media/usb/dvb-usb-v2/mxl111sf.c:74:1: warning: 'mxl111sf_ctrl_msg' uses dynamic stack allocation [enabled by default]
> >
> > Instead, let's enforce a limit for the buffer to be the max size of
> > a control URB payload data (80 bytes).
> >
> > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > Cc: Michael Krufky <mkrufky@kernellabs.com>
> > ---
> >  drivers/media/usb/dvb-usb-v2/mxl111sf.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf.c b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
> > index e97964ef7f56..6538fd54c84e 100644
> > --- a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
> > +++ b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
> > @@ -57,7 +57,12 @@ int mxl111sf_ctrl_msg(struct dvb_usb_device *d,
> >  {
> >         int wo = (rbuf == NULL || rlen == 0); /* write-only */
> >         int ret;
> > -       u8 sndbuf[1+wlen];
> > +       u8 sndbuf[80];
> > +
> > +       if (1 + wlen > sizeof(sndbuf)) {
> > +               pr_warn("%s: len=%d is too big!\n", __func__, wlen);
> > +               return -EREMOTEIO;
> > +       }
> >
> >         pr_debug("%s(wlen = %d, rlen = %d)\n", __func__, wlen, rlen);
> >
> > --
> > 1.8.3.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> I don't really love this, but I see your point. You're right - this
> needs to be fixed.
> 
> AFAIK, the largest transfer the driver ever does is 61 bytes, but I'd
> have to double check to be sure...
> 
> Is there a #define'd macro that we could place there instead of the
> hardcoded '80' ?  I really don't like the number '80' there,
> *especially* not without a comment explaining it.  Is 80 even the
> maximum size of control urb payload data?  Are you sure it isn't 64?
> 
> http://wiki.osdev.org/Universal_Serial_Bus#Maximum_Data_Payload_Size
> 
> ...as per the article above, we should be able to read the actual
> maximum size from the USB endpoint itself, but then again, that would
> leave us with another dynamic static allocation.

There's one driver using 80 bytes for payload (tm6000). Anyway,
I double-checked at USB 2.0 specification: the max size for
control endpoints is 64 bytes for full-speed devices:

	"All Host Controllers are required to have support for 8-, 16-, 32-, and 64-byte maximum data payload sizes
	 for full-speed control endpoints, only 8-byte maximum data payload sizes for low-speed control endpoints,
	 and only 64-byte maximum data payload size for high-speed control endpoints. No Host Controller is
	 required to support larger or smaller maximum data payload sizes."

	Source: USB revision 2.0 - chapter 5.5.3 Control Transfer Packet Size Constraints
		http://www.usb.org/developers/docs/usb_20_070113.zip

So, except for devices that violates that, the worse case scenario is
64 bytes.

It should be noticed that the I2C bus could use a different limit,
so, on PCI devices, in theory, it would be possible to use a larger
window.

Yet, I doubt that any sane tuner/frontend design would require a
packet size bigger than the max size supported by the USB bus, as 
that would limit their usage. Also, most (if not all) of those
tuners/frontends were added due to USB devices, anyway.

> 
> How about if we kzalloc the buffer instead?  (maybe not - that isn't
> very efficient either)

Seems an overkill to me to create/delete a buffer for every single I2C
transfer. Of course, a latter patch could optimize the buffer size to
match what's supported by the hardware, or to use a pre-allocated buffer,
but this is out of my scope: all I want is to get rid of dynamically
allocated buffers. I don't intend to read all those datasheets and
optimize each of those drivers, especially since I may not have the
hardware here for testing.

> If it has to be a static allocation (and it probably should be),
> please #define the size rather than sticking in the number 80.

Ok.

> This feedback applies to your entire "Don't use dynamic static
> allocation" patch series.  Please don't merge those without at least
> #define'ing the size value and adding an appropriate inline comment to
> explain why the maximum is defined as such.

Well, a comment is provided already at the commit message. I don't
see any need to overbloat the code with a comment like that. In any
case, if I were to add a comment, it would be something like: 
	"I guess x bytes would be enough"

As only doing a deep code inspection and reading the datasheets, we'll
know for sure what's the maximum size supported by each device.

Regards,
Mauro