Hello David,
Please pull these firmware patches.
Fixed following Issues and more features can be added :-
1. defined FIRMWARE_NAME so it will easy handling
2. No need to check release_firmware for NON NULL:
if (fw)
release_firmware(fw);
Now we can simply call:
release_firmware(fw);
3. Can do multiple request_firmware but it will use old copy and
return old fw for same FW_NAME and increment count.
And release_firmware will only release_firmware when count
becomes 1 otherwise decrement count.
request_firmware(&fw, FW_NAME, &dev);
request_firmware(&fw, FW_NAME, &dev);
request_firmware(&fw, FW_NAME, &dev);
request_firmware(&fw, FW_NAME, &dev);
release_firmware(fw);
release_firmware(fw);
release_firmware(fw);
release_firmware(fw);
4. Introducing release_firmware_all and release firmware at one short:
request_firmware(&fw, FW_NAME, &dev);
request_firmware(&fw, FW_NAME, &dev);
request_firmware(&fw, FW_NAME, &dev);
request_firmware(&fw, FW_NAME, &dev);
release_firmware_all(fw);
5. No need to check release_firmware_all for NON NULL:
if (fw)
release_firmware_all(fw);
Now we can simply call:
release_firmware_all(fw);
6. Defined firmware handle in structure for handling where ever required.
The following changes since commit 5b664cb235e97afbf34db9c4d77f08ebd725335e:
Linus Torvalds (1):
Merge branch 'upstream-linus' of git://git.kernel.org/.../mfasheh/ocfs2
are available in the git repository at:
git://git.infradead.org/users/jaswinder/firm-jsr-2.6.git master
Jaswinder Singh (18):
firmware: avoiding multiple replication for same firmware file
firmware: convert e100 driver to request_firmware()
firmware: convert acenic driver to request_firmware()
firmware: convert tg3 driver to request_firmware()
firmware: convert av7110 driver to request_firmware()
Remove fdump tool for av7110 firmware
qla1280: use request_firmware
advansys: use request_firmware
qlogicpti: use request_firmware
starfire: use request_firmware()
cassini: use request_firmware
myri_sbus: use request_firmware
tehuti: use request_firmware
typhoon: use request_firmware
smc91c92_cs: use request_firmware
yam: use request_firmware
3C359: use request_firmware
radeon_cp: use request_firmware
drivers/base/firmware_class.c | 156 +-
drivers/gpu/drm/radeon/radeon_cp.c | 151 +-
drivers/gpu/drm/radeon/radeon_drv.h | 6 +
drivers/gpu/drm/radeon/radeon_microcode.h | 1844 -----
drivers/media/dvb/ttpci/Kconfig | 24 +-
drivers/media/dvb/ttpci/Makefile | 9 -
drivers/media/dvb/ttpci/av7110.c | 16 -
drivers/media/dvb/ttpci/av7110_hw.c | 35 +-
drivers/media/dvb/ttpci/av7110_hw.h | 3 +-
drivers/media/dvb/ttpci/fdump.c | 44 -
drivers/net/acenic.c | 122 +-
drivers/net/acenic.h | 4 +
drivers/net/acenic_firmware.h | 9456 -------------------------
drivers/net/cassini.c | 44 +-
drivers/net/cassini.h | 1520 +----
drivers/net/e100.c | 291 +-
drivers/net/hamradio/yam.c | 88 +-
drivers/net/hamradio/yam1200.h | 343 -
drivers/net/hamradio/yam9600.h | 343 -
drivers/net/myri_code.h | 5006 --------------
drivers/net/myri_sbus.c | 45 +-
drivers/net/pcmcia/ositech.h | 358 -
drivers/net/pcmcia/smc91c92_cs.c | 46 +-
drivers/net/starfire.c | 68 +-
drivers/net/starfire_firmware.h | 346 -
drivers/net/starfire_firmware.pl | 31 -
drivers/net/tehuti.c | 43 +-
drivers/net/tehuti.h | 1 +
drivers/net/tehuti_fw.h |10712 -----------------------------
drivers/net/tg3.c | 792 +--
drivers/net/tg3.h | 4 +
drivers/net/tokenring/3c359.c | 53 +-
drivers/net/tokenring/3c359.h | 3 +
drivers/net/tokenring/3c359_microcode.h | 1581 -----
drivers/net/typhoon-firmware.h | 3778 ----------
drivers/net/typhoon.c | 32 +-
drivers/scsi/advansys.c | 1737 +-----
drivers/scsi/ql1040_fw.h | 2130 ------
drivers/scsi/ql12160_fw.h | 1811 -----
drivers/scsi/ql1280_fw.h | 2048 ------
drivers/scsi/qla1280.c | 121 +-
drivers/scsi/qla1280.h | 6 +
drivers/scsi/qlogicpti.c | 65 +-
drivers/scsi/qlogicpti_asm.c | 1160 ----
firmware/3com/3C359.bin.ihex | 1573 +++++
firmware/3com/typhoon.bin.ihex | 2819 ++++++++
firmware/Makefile | 30 +
firmware/WHENCE | 273 +
firmware/acenic/tg1.bin.ihex | 4573 ++++++++++++
firmware/acenic/tg2.bin.ihex | 4844 +++++++++++++
firmware/adaptec/starfire_rx.bin.ihex | 53 +
firmware/adaptec/starfire_tx.bin.ihex | 53 +
firmware/advansys/3550.bin.ihex | 317 +
firmware/advansys/38C0800.bin.ihex | 336 +
firmware/advansys/38C1600.bin.ihex | 398 ++
firmware/advansys/mcode.bin.ihex | 147 +
firmware/av7110/Boot.S | 109 +
firmware/av7110/bootcode.bin.ihex | 15 +
firmware/e100/d101m_ucode.bin.ihex | 38 +
firmware/e100/d101s_ucode.bin.ihex | 38 +
firmware/e100/d102e_ucode.bin.ihex | 38 +
firmware/myricom/lanai.bin.ihex | 4771 +++++++++++++
firmware/ositech/Xilinx7OD.bin.ihex | 177 +
firmware/qlogic/1040.bin.ihex | 2111 ++++++
firmware/qlogic/12160.bin.ihex | 1771 +++++
firmware/qlogic/1280.bin.ihex | 2008 ++++++
firmware/qlogic/isp1000.bin.ihex | 1158 ++++
firmware/radeon/R100_cp.bin.ihex | 130 +
firmware/radeon/R200_cp.bin.ihex | 130 +
firmware/radeon/R300_cp.bin.ihex | 130 +
firmware/radeon/R420_cp.bin.ihex | 130 +
firmware/radeon/R520_cp.bin.ihex | 130 +
firmware/radeon/RS600_cp.bin.ihex | 130 +
firmware/radeon/RS690_cp.bin.ihex | 130 +
firmware/sun/cassini.bin.ihex | 143 +
firmware/tehuti/bdx.bin.ihex | 2678 +++++++
firmware/tigon/tg3.bin.ihex | 175 +
firmware/tigon/tg3_tso.bin.ihex | 446 ++
firmware/tigon/tg3_tso5.bin.ihex | 252 +
firmware/yam/1200.bin.ihex | 342 +
firmware/yam/9600.bin.ihex | 342 +
include/linux/firmware.h | 5 +
82 files changed, 34045 insertions(+), 45374 deletions(-)
delete mode 100644 drivers/gpu/drm/radeon/radeon_microcode.h
delete mode 100644 drivers/media/dvb/ttpci/fdump.c
delete mode 100644 drivers/net/acenic_firmware.h
delete mode 100644 drivers/net/hamradio/yam1200.h
delete mode 100644 drivers/net/hamradio/yam9600.h
delete mode 100644 drivers/net/myri_code.h
delete mode 100644 drivers/net/pcmcia/ositech.h
delete mode 100644 drivers/net/starfire_firmware.h
delete mode 100644 drivers/net/starfire_firmware.pl
delete mode 100644 drivers/net/tehuti_fw.h
delete mode 100644 drivers/net/tokenring/3c359_microcode.h
delete mode 100644 drivers/net/typhoon-firmware.h
delete mode 100644 drivers/scsi/ql1040_fw.h
delete mode 100644 drivers/scsi/ql12160_fw.h
delete mode 100644 drivers/scsi/ql1280_fw.h
delete mode 100644 drivers/scsi/qlogicpti_asm.c
create mode 100644 firmware/3com/3C359.bin.ihex
create mode 100644 firmware/3com/typhoon.bin.ihex
create mode 100644 firmware/acenic/tg1.bin.ihex
create mode 100644 firmware/acenic/tg2.bin.ihex
create mode 100644 firmware/adaptec/starfire_rx.bin.ihex
create mode 100644 firmware/adaptec/starfire_tx.bin.ihex
create mode 100644 firmware/advansys/3550.bin.ihex
create mode 100644 firmware/advansys/38C0800.bin.ihex
create mode 100644 firmware/advansys/38C1600.bin.ihex
create mode 100644 firmware/advansys/mcode.bin.ihex
create mode 100644 firmware/av7110/Boot.S
create mode 100644 firmware/av7110/bootcode.bin.ihex
create mode 100644 firmware/e100/d101m_ucode.bin.ihex
create mode 100644 firmware/e100/d101s_ucode.bin.ihex
create mode 100644 firmware/e100/d102e_ucode.bin.ihex
create mode 100644 firmware/myricom/lanai.bin.ihex
create mode 100644 firmware/ositech/Xilinx7OD.bin.ihex
create mode 100644 firmware/qlogic/1040.bin.ihex
create mode 100644 firmware/qlogic/12160.bin.ihex
create mode 100644 firmware/qlogic/1280.bin.ihex
create mode 100644 firmware/qlogic/isp1000.bin.ihex
create mode 100644 firmware/radeon/R100_cp.bin.ihex
create mode 100644 firmware/radeon/R200_cp.bin.ihex
create mode 100644 firmware/radeon/R300_cp.bin.ihex
create mode 100644 firmware/radeon/R420_cp.bin.ihex
create mode 100644 firmware/radeon/R520_cp.bin.ihex
create mode 100644 firmware/radeon/RS600_cp.bin.ihex
create mode 100644 firmware/radeon/RS690_cp.bin.ihex
create mode 100644 firmware/sun/cassini.bin.ihex
create mode 100644 firmware/tehuti/bdx.bin.ihex
create mode 100644 firmware/tigon/tg3.bin.ihex
create mode 100644 firmware/tigon/tg3_tso.bin.ihex
create mode 100644 firmware/tigon/tg3_tso5.bin.ihex
create mode 100644 firmware/yam/1200.bin.ihex
create mode 100644 firmware/yam/9600.bin.ihex
Thank you,
Jaswinder Singh.
Hello David Dillow,
On Thu, 2008-08-07 at 13:30 -0400, David Dillow wrote:
> On Thu, 2008-08-07 at 22:26 +0530, Jaswinder Singh wrote:
> > firmware: avoiding multiple replication for same firmware file
>
> If this is the last patch you sent to the list, I think there are still
> locking bugs in it. I was short on time, so I didn't give it a full
> review, but I remember seeing problems. I'll try to give it some time
> tonight, if Andrew doesn't beat me to it.
>
This is updated and revised patch.
> > typhoon: use request_firmware
>
> This is untested -- again, no time over the weekend -- and I'd rather
> not have it go upstream until has been verified.
>
>
This is also updated and revised patch.
You can check from :
http://git.infradead.org/users/jaswinder/firm-jsr-2.6.git?a=shortlog
Thank you,
Jaswinder Singh.
On Thu, 2008-08-07 at 22:26 +0530, Jaswinder Singh wrote:
> firmware: avoiding multiple replication for same firmware file
If this is the last patch you sent to the list, I think there are still
locking bugs in it. I was short on time, so I didn't give it a full
review, but I remember seeing problems. I'll try to give it some time
tonight, if Andrew doesn't beat me to it.
> typhoon: use request_firmware
This is untested -- again, no time over the weekend -- and I'd rather
not have it go upstream until has been verified.
On Thu, 2008-08-07 at 23:04 +0530, Jaswinder Singh wrote:
> Hello David Dillow,
>
> On Thu, 2008-08-07 at 13:30 -0400, David Dillow wrote:
> > On Thu, 2008-08-07 at 22:26 +0530, Jaswinder Singh wrote:
> > > firmware: avoiding multiple replication for same firmware file
> >
> > If this is the last patch you sent to the list, I think there are still
> > locking bugs in it. I was short on time, so I didn't give it a full
> > review, but I remember seeing problems. I'll try to give it some time
> > tonight, if Andrew doesn't beat me to it.
>
> This is updated and revised patch.
I just looked at the tree; it still has locking issues, and needs
further review. You must protect the list from modification while you
iterate it looking for an match on the requested firmware. Also, was it
legal to call release_firmware() from an atomic context? It can now
sleep, which may be an issue...
Also, should any two drivers share the same firmware,
release_firmware_all() could leave one of them without its firmware, but
I'm not sure if that is an issue either.
> > > typhoon: use request_firmware
> >
> > This is untested -- again, no time over the weekend -- and I'd rather
> > not have it go upstream until has been verified.
>
> This is also updated and revised patch.
Please drop that patch from the series for now; I'm not happy with it,
as it reintroduces things I've asked be changed. If you get the core
firmware_class issues fixed up, I'll respin what I had.
Dave
Hello Dave,
On Thu, 2008-08-07 at 14:21 -0400, David Dillow wrote:
> On Thu, 2008-08-07 at 23:04 +0530, Jaswinder Singh wrote:
> > Hello David Dillow,
> >
> > On Thu, 2008-08-07 at 13:30 -0400, David Dillow wrote:
> > > On Thu, 2008-08-07 at 22:26 +0530, Jaswinder Singh wrote:
> > > > firmware: avoiding multiple replication for same firmware file
> > >
> > > If this is the last patch you sent to the list, I think there are still
> > > locking bugs in it. I was short on time, so I didn't give it a full
> > > review, but I remember seeing problems. I'll try to give it some time
> > > tonight, if Andrew doesn't beat me to it.
> >
> > This is updated and revised patch.
>
> I just looked at the tree; it still has locking issues, and needs
> further review. You must protect the list from modification while you
> iterate it looking for an match on the requested firmware. Also, was it
> legal to call release_firmware() from an atomic context? It can now
> sleep, which may be an issue...
>
Ok, I will revise it thanks.
> > > > typhoon: use request_firmware
> > >
> > > This is untested -- again, no time over the weekend -- and I'd rather
> > > not have it go upstream until has been verified.
> >
> > This is also updated and revised patch.
>
> Please drop that patch from the series for now; I'm not happy with it,
> as it reintroduces things I've asked be changed.
You mean this :
+ /*
+ * Need to check request_firmware should be called only once
+ * so you don't leak memory if there is more than one NIC.
+ * Need to check if PCI probing gets multi-threaded as
+ * mutex is used while loading the firmware.
+ */
+ if (typhoon_fw != NULL) {
+ err = typhoon_init_firmware(tp);
This is not required now, As I already fixed request_firmware.
So it is replaced by :
+ err = typhoon_init_firmware(tp);
Do you think we still need above comments ?
Thanks you,
Jaswinder Singh.
On Fri, 2008-08-08 at 07:08 +0530, Jaswinder Singh wrote:
> On Thu, 2008-08-07 at 14:21 -0400, David Dillow wrote:
> > Please drop that patch from the series for now; I'm not happy with it,
> > as it reintroduces things I've asked be changed.
>
> You mean this :
>
> + /*
> + * Need to check request_firmware should be called only once
> + * so you don't leak memory if there is more than one NIC.
> + * Need to check if PCI probing gets multi-threaded as
> + * mutex is used while loading the firmware.
> + */
> + if (typhoon_fw != NULL) {
> + err = typhoon_init_firmware(tp);
>
> This is not required now, As I already fixed request_firmware.
>
> So it is replaced by :
>
> + err = typhoon_init_firmware(tp);
>
> Do you think we still need above comments ?
No, the comments will be unneeded, but you don't need an extra function
to handle this, and I'm not real keen about the release_firmware_all()
interface -- it doesn't match up with the get/put semantics of the
reference count.
I don't like releasing the firmware before the pci_unregister_driver()
call. I worry about ordering issues during cleanup, though I'll admit I
have not yet researched if it will be a problem. In any event, if you're
going to request it once per adapter in typhoon_init_one(), then it
should be in the per-device struct, and released in
typhoon_remove_one().
Drop the typhoon patches, and once you fix the problems in the core,
I'll respin the patch in a style I'm comfortable with. It will also need
to be tested before it goes upstream.
Feel free to cc me on the core patches, I will try to review them.
Dave
Hello Dave,
On Thu, 2008-08-07 at 14:21 -0400, David Dillow wrote:
> I just looked at the tree; it still has locking issues, and needs
> further review. You must protect the list from modification while you
> iterate it looking for an match on the requested firmware.
So here is updated patch:
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 6074321..71ec20d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -419,9 +419,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
return -EINVAL;
/* Return firmware pointer from firmware list if already allocated */
+ mutex_lock(&fw_lock);
list_for_each_entry(flst, &firmwarelist, list)
if (strcmp(name, flst->name) == 0) {
- mutex_lock(&fw_lock);
flst->count++;
*firmware_p = flst->fw;
mutex_unlock(&fw_lock);
@@ -429,6 +429,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
name, flst->count);
return 0;
}
+ mutex_unlock(&fw_lock);
*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
if (!firmware)
@@ -568,19 +569,22 @@ void release_firmware(const struct firmware *fw)
{
struct firmware_list *flst;
+ mutex_lock(&fw_lock);
if (fw)
list_for_each_entry(flst, &firmwarelist, list)
if (fw == flst->fw) {
printk(KERN_INFO
"firmware: releasing %s count %d\n",
flst->name, flst->count);
- mutex_lock(&fw_lock);
flst->count--;
- mutex_unlock(&fw_lock);
- if (flst->count == 0)
- __release_firmware(fw, flst);
- return;
+ if (flst->count == 0) {
+ mutex_unlock(&fw_lock);
+ return __release_firmware(fw, flst);
+ }
+ goto out;
}
+out:
+ mutex_unlock(&fw_lock);
}
/*
@@ -598,6 +602,7 @@ void release_firmware_all(const struct firmware *fw)
{
struct firmware_list *flst;
+ mutex_lock(&fw_lock);
if (fw)
list_for_each_entry(flst, &firmwarelist, list)
if (fw == flst->fw) {
@@ -605,13 +610,14 @@ void release_firmware_all(const struct firmware *fw)
"firmware: release_all %s count %d\n",
flst->name, flst->count);
if (flst->count) {
- mutex_lock(&fw_lock);
flst->count = 0;
mutex_unlock(&fw_lock);
- __release_firmware(fw, flst);
+ return __release_firmware(fw, flst);
}
- return;
+ goto out;
}
+out:
+ mutex_unlock(&fw_lock);
}
/* Async support */
> Also, was it legal to call release_firmware() from an atomic context? It can now
> sleep, which may be an issue...
>
yes, release_firmware can sleep.
So now release_firmware also joined the family of request_firmware.
If you can do request_firmware you can also do release_firmware.
Any how release_firmware will be called below request_firmware or during
exit, I do not think this will make any issue.
Thank you,
Jaswinder Singh.
Hello Dave,
On Thu, 2008-08-07 at 22:59 -0400, David Dillow wrote:
> >
> > Do you think we still need above comments ?
>
> No, the comments will be unneeded, but you don't need an extra function
> to handle this, and I'm not real keen about the release_firmware_all()
> interface -- it doesn't match up with the get/put semantics of the
> reference count.
>
> I don't like releasing the firmware before the pci_unregister_driver()
> call. I worry about ordering issues during cleanup, though I'll admit I
> have not yet researched if it will be a problem. In any event, if you're
> going to request it once per adapter in typhoon_init_one(), then it
> should be in the per-device struct, and released in
> typhoon_remove_one().
>
Here is updated patch :
diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index 2a26ba5..1638e87 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -2615,6 +2615,9 @@ typhoon_remove_one(struct pci_dev *pdev)
pci_set_power_state(pdev, PCI_D0);
pci_restore_state(pdev);
typhoon_reset(tp->ioaddr, NoWait);
+
+ release_firmware(typhoon_fw);
+
pci_iounmap(pdev, tp->ioaddr);
pci_free_consistent(pdev, sizeof(struct typhoon_shared),
tp->shared, tp->shared_dma);
@@ -2645,8 +2648,6 @@ typhoon_init(void)
static void __exit
typhoon_cleanup(void)
{
- release_firmware_all(typhoon_fw);
-
pci_unregister_driver(&typhoon_driver);
}
> Drop the typhoon patches, and once you fix the problems in the core,
> I'll respin the patch in a style I'm comfortable with. It will also need
> to be tested before it goes upstream.
>
I can understand you are very worried about typhoon.
But this is only first version of patches. This will goto David
WoodHouse tree and he will again revise it.
And driver is yours you can change it as per your comfort, No one can
stop you or typhoon ;)
Thank you,
Jaswinder Singh.
On Fri, 2008-08-08 at 09:01 +0530, Jaswinder Singh wrote:
> On Thu, 2008-08-07 at 14:21 -0400, David Dillow wrote:
> > I just looked at the tree; it still has locking issues, and needs
> > further review. You must protect the list from modification while you
> > iterate it looking for an match on the requested firmware.
>
> So here is updated patch:
I'll take a closer look when I'm awake, but there are some nitpicky
style issues remaining:
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 6074321..71ec20d 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -568,19 +569,22 @@ void release_firmware(const struct firmware *fw)
> {
> struct firmware_list *flst;
>
> + mutex_lock(&fw_lock);
> if (fw)
> list_for_each_entry(flst, &firmwarelist, list)
> if (fw == flst->fw) {
> printk(KERN_INFO
> "firmware: releasing %s count %d\n",
> flst->name, flst->count);
> - mutex_lock(&fw_lock);
> flst->count--;
> - mutex_unlock(&fw_lock);
> - if (flst->count == 0)
> - __release_firmware(fw, flst);
> - return;
> + if (flst->count == 0) {
> + mutex_unlock(&fw_lock);
> + return __release_firmware(fw, flst);
> + }
> + goto out;
> }
> +out:
> + mutex_unlock(&fw_lock);
> }
You don't need the 'goto out', a break will work fine. And you'll not be
pressed up against the right side of the screen if you just do
if (!fw)
return;
at the top of the function.
> @@ -598,6 +602,7 @@ void release_firmware_all(const struct firmware *fw)
I still don't like this exception to the get/put ref-counting. Is this
used anywhere else in your series, or was typhoon the only one?
> > Also, was it legal to call release_firmware() from an atomic context? It can now
> > sleep, which may be an issue...
>
> yes, release_firmware can sleep.
> So now release_firmware also joined the family of request_firmware.
The question wasn't if it can sleep now, it was if it could sleep before
you started changing it. I now know that it has always called vfree(),
so it has always needed to be able to sleep.
> Any how release_firmware will be called below request_firmware or during
> exit, I do not think this will make any issue.
I need to run down code to see if my thoughts are realistic, but say
eth0 was a typhoon:
modprobe typhoon
ip link set eth0 up
rmmod typhoon
<firmware unloaded>
<sleep in typhoon_remove_one() waiting for 'ip link set eth0 down'>
<Tx timeout, needing to reset and reload firmware>
Boom.
Dave
On Fri, 2008-08-08 at 09:09 +0530, Jaswinder Singh wrote:
> On Thu, 2008-08-07 at 22:59 -0400, David Dillow wrote:
> > I don't like releasing the firmware before the pci_unregister_driver()
> > call. I worry about ordering issues during cleanup, though I'll admit I
> > have not yet researched if it will be a problem. In any event, if you're
> > going to request it once per adapter in typhoon_init_one(), then it
> > should be in the per-device struct, and released in
> > typhoon_remove_one().
>
> Here is updated patch :
You've fixed one part of the issues I described, but I think you've left
open the possibility of dangling pointers when one card in a multi NIC
system fails to come up. Again, I need to track down the code to see if
I'm all wet or not.
> > Drop the typhoon patches, and once you fix the problems in the core,
> > I'll respin the patch in a style I'm comfortable with. It will also need
> > to be tested before it goes upstream.
>
> I can understand you are very worried about typhoon.
> But this is only first version of patches. This will goto David
> WoodHouse tree and he will again revise it.
>
> And driver is yours you can change it as per your comfort, No one can
> stop you or typhoon ;)
I'd rather not have to undo the breakage when I fix it to my comfort --
best to get it right the first time if it is possible, and it is in this
instance. I'm not just worried about typhoon, I'm worried about the core
code and the other drivers you've converted. A brief skim over your tg3
and acenic patches suggests you took to heart the need to keep the
firmware around while the device is up, but the basic locking issues in
the core code suggest there are more lessons to be absorbed.
There's no way to get experience except by doing, and I applaud your
efforts to learn the code, but please drop the typhoon part and
concentrate on getting the core right. I've already committed to fixing
typhoon once you do.
Dave
Hello Dave,
On Fri, 2008-08-08 at 00:10 -0400, David Dillow wrote:
> I'll take a closer look when I'm awake, but there are some nitpicky
> style issues remaining:
>
Currently I use checkpatch.pl scripts for styles problem:
#./scripts/checkpatch.pl 00*
total: 0 errors, 0 warnings, 241 lines checked
0001-firmware-avoiding-multiple-replication-for-same-fir.patch has no obvious style problems and is ready for submission.
Please give me your script I will try with your scripts.
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 6074321..71ec20d 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
>
> > @@ -568,19 +569,22 @@ void release_firmware(const struct firmware *fw)
> > {
> > struct firmware_list *flst;
> >
> > + mutex_lock(&fw_lock);
> > if (fw)
> > list_for_each_entry(flst, &firmwarelist, list)
> > if (fw == flst->fw) {
> > printk(KERN_INFO
> > "firmware: releasing %s count %d\n",
> > flst->name, flst->count);
> > - mutex_lock(&fw_lock);
> > flst->count--;
> > - mutex_unlock(&fw_lock);
> > - if (flst->count == 0)
> > - __release_firmware(fw, flst);
> > - return;
> > + if (flst->count == 0) {
> > + mutex_unlock(&fw_lock);
> > + return __release_firmware(fw, flst);
> > + }
> > + goto out;
> > }
> > +out:
> > + mutex_unlock(&fw_lock);
> > }
>
> You don't need the 'goto out', a break will work fine.
Earlier I was also using break.
For a safe side I used goto as If some one write more code it will not
make any problem.
> And you'll not be
> pressed up against the right side of the screen if you just do
> if (!fw)
> return;
> at the top of the function.
>
Yes, I also think about this. But this is not an issue as it is < 80
and every thing is coming in one line only.
> > @@ -598,6 +602,7 @@ void release_firmware_all(const struct firmware *fw)
>
> I still don't like this exception to the get/put ref-counting. Is this
> used anywhere else in your series, or was typhoon the only one?
>
> > > Also, was it legal to call release_firmware() from an atomic context? It can now
> > > sleep, which may be an issue...
> >
> > yes, release_firmware can sleep.
> > So now release_firmware also joined the family of request_firmware.
>
> The question wasn't if it can sleep now, it was if it could sleep before
> you started changing it. I now know that it has always called vfree(),
> so it has always needed to be able to sleep.
>
> > Any how release_firmware will be called below request_firmware or during
> > exit, I do not think this will make any issue.
>
> I need to run down code to see if my thoughts are realistic, but say
> eth0 was a typhoon:
>
> modprobe typhoon
> ip link set eth0 up
> rmmod typhoon
> <firmware unloaded>
> <sleep in typhoon_remove_one() waiting for 'ip link set eth0 down'>
> <Tx timeout, needing to reset and reload firmware>
> Boom.
David Woodhouse what you think about this.
Thank you,
Jaswinder Singh.
Hello Dave,
On Fri, 2008-08-08 at 00:25 -0400, David Dillow wrote:
> On Fri, 2008-08-08 at 09:09 +0530, Jaswinder Singh wrote:
> > On Thu, 2008-08-07 at 22:59 -0400, David Dillow wrote:
> > > I don't like releasing the firmware before the pci_unregister_driver()
> > > call. I worry about ordering issues during cleanup, though I'll admit I
> > > have not yet researched if it will be a problem. In any event, if you're
> > > going to request it once per adapter in typhoon_init_one(), then it
> > > should be in the per-device struct, and released in
> > > typhoon_remove_one().
> >
> > Here is updated patch :
>
> You've fixed one part of the issues I described, but I think you've left
> open the possibility of dangling pointers when one card in a multi NIC
> system fails to come up. Again, I need to track down the code to see if
> I'm all wet or not.
>
That's why I introduced firmware_release_all() for buggy drivers.
I think firmware_release_all() is best for you case.
Thank you,
Jaswinder Singh.
On Fri, 2008-08-08 at 10:03 +0530, Jaswinder Singh wrote:
> On Fri, 2008-08-08 at 00:10 -0400, David Dillow wrote:
>
> > I'll take a closer look when I'm awake, but there are some nitpicky
> > style issues remaining:
>
> Please give me your script I will try with your scripts.
I am afraid I can send you neither my eyes nor my gray matter. Loaning
the use of them is another matter, but you've had them at least
semi-engaged for some time now. :)
The things I'm pointing out may be nitpicky, yes, but they are things
you get from looking at the shape of the code and seeing ways that it
could be made simpler and more readable.
> > You don't need the 'goto out', a break will work fine.
>
> Earlier I was also using break.
> For a safe side I used goto as If some one write more code it will not
> make any problem.
So the person who adds more code can change to a goto at that time --
meanwhile, the code will be more readable.
> > And you'll not be
> > pressed up against the right side of the screen if you just do
>
> Yes, I also think about this. But this is not an issue as it is < 80
> and every thing is coming in one line only.
Is the wrapping on the printk not just butt-ugly to you? If it can be
easily avoided by removing some indent, why wouldn't you want to?
These are small points, to be sure.
Dave