lkml.org 
[lkml]   [2008]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [GIT PULL]: firmware patches for building firmware into kernel
From
Date
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/

\
 
 \ /
  Last update: 2008-08-08 06:39    [W:0.105 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site