[GIT PULL]: firmware patches for building firmware into kernel - Kernel

This is a discussion on [GIT PULL]: firmware patches for building firmware into kernel - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 13 of 13

Thread: [GIT PULL]: firmware patches for building firmware into kernel

  1. [GIT PULL]: firmware patches for building firmware into kernel

    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.

    --
    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/

  2. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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/jaswi...git?a=shortlog

    Thank you,

    Jaswinder Singh.


    --
    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/

  3. Re: [GIT PULL]: firmware patches for building firmware into kernel


    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.


    --
    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/

  4. Re: [GIT PULL]: firmware patches for building firmware into kernel


    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
    --
    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/

  5. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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.

    --
    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/

  6. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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

    --
    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/

  7. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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.


    --
    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/

  8. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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.

    --
    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/

  9. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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



    Boom.

    Dave

    --
    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/

  10. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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

    --
    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/

  11. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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
    >
    >
    >
    > Boom.


    David Woodhouse what you think about this.

    Thank you,

    Jaswinder Singh.

    --
    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/

  12. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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.

    --
    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/

  13. Re: [GIT PULL]: firmware patches for building firmware into kernel

    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



    --
    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/

+ Reply to Thread