lkml.org 
[lkml]   [2005]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] Add disk hotswap support to libata RESEND #5
On 9/28/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> > + PDC2_SATA_PLUG_CSR = 0X60, /* SATAII Plug control/status reg */
>
> Did you actually compile and test this? :)

Yes. And it works, which is why the compiling and testing doesn't
catch it :P I'll change it.

> > + case board_40518:
> > + /* Override hotplug offset for SATAII150 */
> > + hp->hotplug_offset = PDC2_SATA_PLUG_CSR;
>
> add a comment /* fall through */ here

OK.

> > - /* FIXME: check ata_device_add return value */
> > + /* FIXME: check ata_device_add return value. If 0, kfree(hp) */
> > ata_device_add(probe_ent);
>
> Just leave the comment as is. You made it worse:
>
> * if ata_device_add() returns zero, then everything is OK.
>
> * if ata_device_add() returns non-zero, then an error occured.
> kfree(hp) is but one of several things that need to be cleaned up on
> failure.

No. ata_device_add returns nonzero on success; so say the docs.
Since the return value is not checked here, and whether on success or
failure all of the data structures allocated in that method stick
around, I assumed that something was in the works for this. I'll
change this to kfree(hp) on returning 0. Please advise if I should do
something else.

> Finally, please fix the format of your subject line per
> http://linux.yyz.us/patch-format.html
>
> Most notably, each Subject should be unique for each patch. e.g.
>
> [PATCH 1/3] sata_promise: fix hotplug register offset
> [PATCH 2/3] libata: add device hotplug infrastructure
> [PATCH 3/3] sata_promise: add device hotplug support

OK.

Luke
-
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: 2005-09-28 21:49    [W:2.059 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site