Messages in this thread | | | Subject | Re: [GIT PULL]: firmware patches for building firmware into kernel | From | Jaswinder Singh <> | Date | Fri, 08 Aug 2008 10:03:24 +0530 |
| |
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.
-- 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/
| |