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

[matthieu castet <castet.matthieu@free.fr>]

Hi Andrew,

thanks for the review.

Andrew Morton wrote:
> matthieu castet <castet.matthieu@free.fr> 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