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

[Greg KH <greg@kroah.com>]

On Wed, Nov 02, 2005 at 09:15:58PM +0100, matthieu castet wrote:
> Hi Greg,
> 
> thanks for your review.
> 
> Greg KH wrote:
> >On Sun, Oct 30, 2005 at 12:37:41AM +0200, matthieu castet wrote:
> >
> >>Please comment and consider for inclusion.
> >
> >
> >I need a "Signed-off-by:" line in order to be able to add it.  Care to
> >redo things based on the comments you have had and resend it with this
> >line?
> >
> >
> no problem ;)
> >>+ *
> >>+ * This software is available to you under a choice of one of two
> >>+ * licenses. You may choose to be licensed under the terms of the GNU
> >>+ * General Public License (GPL) Version 2, available from the file
> >>+ * COPYING in the main directory of this source tree, or the
> >>+ * BSD license below:
> >>+ *
> >>+ * Redistribution and use in source and binary forms, with or without
> >>+ * modification, are permitted provided that the following conditions
> >>+ * are met:
> >
> >
> ><snip>  You don't need the whole GPL 2 copy here, just put the first
> >paragraph you have before this one in.
> >
> The paragraph you quote is the BSD licence, and point 1 is :
> Redistributions of source code must retain the above copyright
>  *    notice unmodified, this list of conditions, and the following
>  *    disclaimer
> 
> So could I remove it ?

Ugh, you are right, I missed that this was BSD also, sorry.  It's fine.

> >>+/*
> >>+ * sometime hotplug don't have time to give the firmware the
> >>+ * first time, retry it.
> >>+ */
> >>+static int sleepy_request_firmware(const struct firmware **fw, 
> >>+		const char *name, struct device *dev)
> >>+{
> >>+	if (request_firmware(fw, name, dev) == 0)
> >>+		return 0;
> >>+	msleep(1000);
> >>+	return request_firmware(fw, name, dev);
> >>+}
> >
> >
> >No, use the async firmware download mode instead of this.  That will
> >solve all of your problems.
> >
> >
> Thanks, but does userspace will retry if it fails the first time ?
> The device needs the firmware quickly and after 3-5 seconds without it, 
> it goes berserk.

That sounds like a pretty broken device :)

I don't know, have your own timer that disconnects it after X seconds if
you haven't gotten the firmware yet?  Don't rely on userspace getting
around to you real quickly, as we have seen lots of problems with
drivers relying on that timeout.

> >>+/* we need to use semaphore until sysfs and removable devices is fixed
> >>+ * the problem is explained on 
> >>http://marc.theaimsgroup.com/?t=112006484100003
> >>+ */
> >
> >
> >This is the proper fix, why do you think it should be fixed in the
> >driver core?
> >
> I don't remember, but aren't any possible race in sysfs code ?
> In the read, after the up, the usb_disconnect is scheduled, and call 
> device_remove_file. Is that ok for the sysfs code to be still in the 
> read code ?

Depends on what that code does.

> An other case : a process open a file, and start a read. Before the down 
> , the usb_disconnect is scheduled and the module is removed.
> What will do the read, run code from the removed code ?

Again, depends on how your code is structured, and what you are
referencing in that read code.

> >>diff -rNu -x '*.ko*' -x '*.mod*' -x '*.o*' 
> >>linux-2.6.14/drivers/usb/atm.old/ueagle-atm.h 
> >>linux-2.6.14/drivers/usb/atm/ueagle-atm.h
> >>--- linux-2.6.14/drivers/usb/atm.old/ueagle-atm.h	1970-01-01 
> >>01:00:00.000000000 +0100
> >>+++ linux-2.6.14/drivers/usb/atm/ueagle-atm.h	2005-10-30 
> >>00:25:27.000000000 +0200
> >
> >
> >Why do you need a header file for a single .c file?
> >
> I think it makes things cleaner. I even like the bsd style where there 
> is an header for reg (hardware values) and an other for val (driver 
> structures).

This isn't BSD :)

thanks,

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