On Wed, Sep 12, 2012 at 05:49:23PM +0800, Macpaul Lin wrote: > 1. Replace file I/O of reading firmware by request_firmware(). > 2. Coding style clean up. > This should be two patches. It makes it easier to discuss the parts. There is a bug in em_download_image() so these patches will need to be redone. The comments don't all need to be addressed. They are just comments for if someone wants to do further cleanup work. > Signed-off-by: Macpaul Lin <macpaul@xxxxxxxxx> > Cc: Paul Stewart <pstew@xxxxxxxxxxxx> > Cc: Ben Chan <benchan@xxxxxxxxxxxx> > Cc: Sage Ahn <syahn@xxxxxxxxxxx> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > --- > drivers/staging/gdm72xx/usb_boot.c | 207 ++++++++++++++++++------------------- > 1 file changed, 99 insertions(+), 108 deletions(-) > > diff --git a/drivers/staging/gdm72xx/usb_boot.c b/drivers/staging/gdm72xx/usb_boot.c > index e3dbd5a..e1cd017 100644 > --- a/drivers/staging/gdm72xx/usb_boot.c > +++ b/drivers/staging/gdm72xx/usb_boot.c > @@ -18,25 +18,29 @@ > #include <linux/usb.h> > #include <linux/unistd.h> > #include <linux/slab.h> > +#include <linux/firmware.h> > > #include <asm/byteorder.h> > #include "gdm_usb.h" > #include "usb_boot.h" > > -#define DN_KERNEL_MAGIC_NUMBER 0x10760001 > -#define DN_ROOTFS_MAGIC_NUMBER 0x10760002 > +#define DN_KERNEL_MAGIC_NUMBER 0x10760001 > +#define DN_ROOTFS_MAGIC_NUMBER 0x10760002 > > -#define DOWNLOAD_SIZE 1024 > +#define DOWNLOAD_SIZE 1024 > > -#define DH2B(x) __cpu_to_be32(x) > -#define DL2H(x) __le32_to_cpu(x) > +#define DH2B(x) __cpu_to_be32(x) > +#define DL2H(x) __le32_to_cpu(x) We should get rid of these btw and use cpu_to_be32() directly. Including be32 annotations and testing with Sparse. http://lwn.net/Articles/205624/ Doesn't have to be in one patch. The problem is that since you combined white space fixes with other fixes we end up discussing this, instead of just taking the other fix. > > -#define MIN(a, b) ((a) > (b) ? (b) : (a)) > +#define MIN(a, b) ((a) > (b) ? (b) : (a)) > The kernel has its on min and max macros. Use those. Again, if this line had been left out of the original patch, I wouldn't be discussing it... So make your patches minimal and it's less hassle. > #define MAX_IMG_CNT 16 > -#define UIMG_PATH "/lib/firmware/gdm72xx/gdmuimg.bin" > -#define KERN_PATH "/lib/firmware/gdm72xx/zImage" > -#define FS_PATH "/lib/firmware/gdm72xx/ramdisk.jffs2" > + > +#define FW_DIR "gdm72xx/" > + > +#define FW_UIMG "gdmuimg.bin" > +#define FW_KERN "zImage" > +#define FW_FS "ramdisk.jffs2" > > struct dn_header { > u32 magic_num; > @@ -44,23 +48,23 @@ struct dn_header { > }; > > struct img_header { > - u32 magic_code; > - u32 count; > - u32 len; > - u32 offset[MAX_IMG_CNT]; > + u32 magic_code; > + u32 count; > + u32 len; > + u32 offset[MAX_IMG_CNT]; > char hostname[32]; > char date[32]; > }; > > struct fw_info { > - u32 id; > - u32 len; > - u32 kernel_len; > - u32 rootfs_len; > - u32 kernel_offset; > - u32 rootfs_offset; > - u32 fw_ver; > - u32 mac_ver; > + u32 id; > + u32 len; > + u32 kernel_len; > + u32 rootfs_len; > + u32 kernel_offset; > + u32 rootfs_offset; > + u32 fw_ver; > + u32 mac_ver; > char hostname[32]; > char userid[16]; > char date[32]; > @@ -107,13 +111,13 @@ static int gdm_wibro_recv(struct usb_device *usbdev, void *data, int len) > return 0; > } > > -static int download_image(struct usb_device *usbdev, struct file *filp, > - loff_t *pos, u32 img_len, u32 magic_num) > +static int download_image(struct usb_device *usbdev, > + const struct firmware *firm, > + loff_t pos, u32 img_len, u32 magic_num) > { > struct dn_header h; > int ret = 0; > u32 size; be32 size; > - int len, readn; > > size = (img_len + DOWNLOAD_SIZE - 1) & ~(DOWNLOAD_SIZE - 1); size = ALIGN(img_len, DOWNLOAD_SIZE); > h.magic_num = DH2B(magic_num); > @@ -123,43 +127,43 @@ static int download_image(struct usb_device *usbdev, struct file *filp, > if (ret < 0) > goto out; > > - readn = 0; > - while ((len = filp->f_op->read(filp, tx_buf, DOWNLOAD_SIZE, pos))) { > - > - if (len < 0) { > - ret = -1; > + while (img_len > 0) { > + if (img_len > DOWNLOAD_SIZE) { > + memcpy(tx_buf, firm->data + pos, DOWNLOAD_SIZE); > + ret = gdm_wibro_send(usbdev, tx_buf, DOWNLOAD_SIZE); Move the test for failure here so it's next to the call we're testing. if (ret) return ret; > + } else { /* the last chuch of data */ > + memcpy(tx_buf, firm->data + pos, img_len); > + ret = gdm_wibro_send(usbdev, tx_buf, img_len); > goto out; Return directly. Don't bunny hop to out. > } > - readn += len; > > - ret = gdm_wibro_send(usbdev, tx_buf, DOWNLOAD_SIZE); > + img_len -= DOWNLOAD_SIZE; > + pos += DOWNLOAD_SIZE; > + > if (ret < 0) > goto out; > - if (readn >= img_len) > - break; > } > > - if (readn < img_len) { > - printk(KERN_ERR "gdmwm: Cannot read to the requested size. " > - "Read = %d Requested = %d\n", readn, img_len); > - ret = -EIO; > - } > out: > - > return ret; > } > > int usb_boot(struct usb_device *usbdev, u16 pid) > { > int i, ret = 0; > - struct file *filp = NULL; > - struct inode *inode = NULL; > - static mm_segment_t fs; > struct img_header hdr; > struct fw_info fw_info; > loff_t pos = 0; > - char *img_name = UIMG_PATH; > - int len; > + char *img_name = FW_DIR FW_UIMG; > + const struct firmware *firm; > + > + ret = request_firmware(&firm, img_name, &usbdev->dev); > + if (ret < 0) { > + printk(KERN_ERR > + "requesting firmware %s failed with error %d\n", > + img_name, ret); > + return ret; > + } > > tx_buf = kmalloc(DOWNLOAD_SIZE, GFP_KERNEL); > if (tx_buf == NULL) { > @@ -167,29 +171,12 @@ int usb_boot(struct usb_device *usbdev, u16 pid) > return -ENOMEM; > } > > - fs = get_fs(); > - set_fs(get_ds()); > - > - filp = filp_open(img_name, O_RDONLY | O_LARGEFILE, 0); > - if (IS_ERR(filp)) { > - printk(KERN_ERR "Can't find %s.\n", img_name); > - ret = PTR_ERR(filp); > - goto restore_fs; > - } > - > - inode = filp->f_dentry->d_inode; > - if (!S_ISREG(inode->i_mode)) { > - printk(KERN_ERR "Invalid file type: %s\n", img_name); > - ret = -EINVAL; > - goto out; > - } > - > - len = filp->f_op->read(filp, (u8 *)&hdr, sizeof(hdr), &pos); > - if (len != sizeof(hdr)) { > + if (firm->size < sizeof(hdr)) { > printk(KERN_ERR "gdmwm: Cannot read the image info.\n"); > ret = -EIO; > goto out; > } > + memcpy((u8 *)&hdr, firm->data, sizeof(hdr)); ^^^^^^ Don't need to cast this. > > array_le32_to_cpu((u32 *)&hdr, 19); > #if 0 > @@ -217,13 +204,12 @@ int usb_boot(struct usb_device *usbdev, u16 pid) > } > > pos = hdr.offset[i]; > - len = filp->f_op->read(filp, (u8 *)&fw_info, sizeof(fw_info), > - &pos); > - if (len != sizeof(fw_info)) { > + if (firm->size < sizeof(fw_info) + pos) { > printk(KERN_ERR "gdmwm: Cannot read the FW info.\n"); > ret = -EIO; > goto out; > } > + memcpy((u8 *)&fw_info, firm->data + pos, sizeof(fw_info)); ^^^^^^ No need. > > array_le32_to_cpu((u32 *)&fw_info, 8); > #if 0 > @@ -239,14 +225,23 @@ int usb_boot(struct usb_device *usbdev, u16 pid) > continue; > > pos = hdr.offset[i] + fw_info.kernel_offset; > - ret = download_image(usbdev, filp, &pos, fw_info.kernel_len, > - DN_KERNEL_MAGIC_NUMBER); > + if (firm->size < fw_info.kernel_len + pos) { > + printk(KERN_ERR "gdmwm: Kernel FW is too small.\n"); > + goto out; > + } > + > + ret = download_image(usbdev, firm, pos, > + fw_info.kernel_len, DN_KERNEL_MAGIC_NUMBER); > if (ret < 0) > goto out; > printk(KERN_INFO "GCT: Kernel download success.\n"); > > pos = hdr.offset[i] + fw_info.rootfs_offset; > - ret = download_image(usbdev, filp, &pos, fw_info.rootfs_len, > + if (firm->size < fw_info.rootfs_len + pos) { > + printk(KERN_ERR "gdmwm: Filesystem FW is too small.\n"); > + goto out; > + } > + ret = download_image(usbdev, firm, pos, fw_info.rootfs_len, > DN_ROOTFS_MAGIC_NUMBER); > if (ret < 0) > goto out; > @@ -260,10 +255,8 @@ int usb_boot(struct usb_device *usbdev, u16 pid) > ret = -EINVAL; > } > out: > - filp_close(filp, NULL); > - > -restore_fs: > - set_fs(fs); > + release_firmware(firm); > + firm = NULL; No point in setting stack variable to NULL right before a return. > kfree(tx_buf); > return ret; > } > @@ -293,38 +286,27 @@ out: > return ret; > } > > -static int em_download_image(struct usb_device *usbdev, char *path, > +static int em_download_image(struct usb_device *usbdev, char *img_name, > char *type_string) > { > - struct file *filp; > - struct inode *inode; > - static mm_segment_t fs; > char *buf = NULL; > loff_t pos = 0; > int ret = 0; > - int len, readn = 0; > + int len = 0; ^^^^^^^^^^^ len is always zero in this function... :( > + int img_len = 0; ^^^^^^^^^^^^^^^ This initialization is not needed. > + const struct firmware *firm; > #if defined(GDM7205_PADDING) > const int pad_size = GDM7205_PADDING; > #else > const int pad_size = 0; > #endif > > - fs = get_fs(); > - set_fs(get_ds()); > - > - filp = filp_open(path, O_RDONLY | O_LARGEFILE, 0); > - if (IS_ERR(filp)) { > - printk(KERN_ERR "Can't find %s.\n", path); > - set_fs(fs); > - ret = -ENOENT; > - goto restore_fs; > - } > - > - inode = filp->f_dentry->d_inode; > - if (!S_ISREG(inode->i_mode)) { > - printk(KERN_ERR "Invalid file type: %s\n", path); > - ret = -EINVAL; > - goto out; > + ret = request_firmware(&firm, img_name, &usbdev->dev); > + if (ret < 0) { > + printk(KERN_ERR > + "requesting firmware %s failed with error %d\n", > + img_name, ret); > + return ret; > } > > buf = kmalloc(DOWNLOAD_CHUCK + pad_size, GFP_KERNEL); > @@ -338,15 +320,25 @@ static int em_download_image(struct usb_device *usbdev, char *path, > if (ret < 0) > goto out; > > - while ((len = filp->f_op->read(filp, buf+pad_size, DOWNLOAD_CHUCK, > - &pos))) { > - if (len < 0) { > - ret = -1; > - goto out; > + img_len = firm->size; > + > + if (img_len <= 0) { > + ret = -1; > + goto out; > + } > + > + while (img_len > 0) { > + if (img_len > DOWNLOAD_CHUCK) { > + memcpy(buf+pad_size, firm->data + pos, DOWNLOAD_CHUCK); > + ret = gdm_wibro_send(usbdev, buf, len+pad_size); ^^^^^^^^^^^^ This isn't right. > + } else { /* the last chuch of data */ > + memcpy(buf+pad_size, firm->data + pos, img_len); > + ret = gdm_wibro_send(usbdev, buf, len+pad_size); ^^^^^^^^^^^^ Should be img_len or something. This should mirror the function above. The other comments apply. Return here. > } > - readn += len; > > - ret = gdm_wibro_send(usbdev, buf, len+pad_size); > + img_len -= DOWNLOAD_CHUCK; > + pos += DOWNLOAD_CHUCK; > + > if (ret < 0) > goto out; > > @@ -360,11 +352,8 @@ static int em_download_image(struct usb_device *usbdev, char *path, > goto out; > > out: > - filp_close(filp, NULL); > - > -restore_fs: > - set_fs(fs); > - > + release_firmware(firm); > + firm = NULL; Not needed. > kfree(buf); > > return ret; > @@ -382,13 +371,15 @@ static int em_fw_reset(struct usb_device *usbdev) > int usb_emergency(struct usb_device *usbdev) > { > int ret; > + char *kern_name = FW_DIR FW_KERN; > + char *fs_name = FW_DIR FW_FS; > > - ret = em_download_image(usbdev, KERN_PATH, KERNEL_TYPE_STRING); > + ret = em_download_image(usbdev, kern_name, KERNEL_TYPE_STRING); > if (ret < 0) > goto out; > printk(KERN_INFO "GCT Emergency: Kernel download success.\n"); > > - ret = em_download_image(usbdev, FS_PATH, FS_TYPE_STRING); > + ret = em_download_image(usbdev, fs_name, FS_TYPE_STRING); > if (ret < 0) > goto out; > printk(KERN_INFO "GCT Emergency: Filesystem download success.\n"); > -- > 1.7.11.2.138.g2b53359 > > _______________________________________________ > devel mailing list > devel@xxxxxxxxxxxxxxxxxxxxxx > http://driverdev.linuxdriverproject.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
