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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andrew,

thanks for the review.

Andrew Morton wrote:
matthieu castet <[email protected]> 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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux