Re: [PATCH] Eagle and ADI 930 usb adsl modem driver

[matthieu castet]

Hi Andrew,

thanks for the review.

Andrew Morton wrote:
> matthieu castet <castet.matthieu@xxxxxxx> wrote:

> > ...
> > +static int request_cmvs(struct uea_softc *sc,
> > +		 struct uea_cmvs **cmvs, const struct firmware **fw)
> > +{
> > +	int ret, size;
> > +	u8 *data;
> > +	char *file;
> > +	char cmv_name[256] = FW_DIR;
>
>
> That's rather a lot of stack.  Can this be made static, of kmalloced?
I think we'll made it static.

> > +
> > +	*cmvs = (struct uea_cmvs *)(data + 1);
>
>
> That's a bit rude - asking the compiler to perform a structure copy from an
> odd address.  memcpy() would be saner.
Could you elaborate a bit more ?
I don't see where there is a copy.
*cmvs is a pointer to the structure, not the structure. And when we 
parse the structure, we use get_unaligned functions.


> > ...
> > +/**
> > + * uea_read_proc : /proc information
> > + */
> > +static int uea_read_proc(char *page, 
> > +		char **start, off_t off, int count, int *eof, void *data)
>
>
> People get shouted at for adding /proc handlers.  Greg may have thoughts...
Ok, we may be convert some values to sysfs. It would be nice if usbatm 
allow us to export some common value (margin, ...).


Matthieu
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/