Hey Jeff, all,
Patch 1/3 for libata hotswapping, properly masking out hotplug
interrupts on Promise SATAII150 line of controllers.
More comments available in patch and header, please review and apply
if you like it!
Luke Kosewski
Lukasz Kosewski wrote:
> @@ -57,6 +57,7 @@ enum {
> PDC_GLOBAL_CTL = 0x48, /* Global control/status (per port) */
> PDC_CTLSTAT = 0x60, /* IDE control and status (per port) */
> PDC_SATA_PLUG_CSR = 0x6C, /* SATA Plug control/status reg */
> + PDC2_SATA_PLUG_CSR = 0X60, /* SATAII Plug control/status reg */
Did you actually compile and test this? :)
> @@ -690,6 +745,9 @@ static int pdc_ata_init_one (struct pci_
>
> /* notice 4-port boards */
> switch (board_idx) {
> + case board_40518:
> + /* Override hotplug offset for SATAII150 */
> + hp->hotplug_offset = PDC2_SATA_PLUG_CSR;
add a comment /* fall through */ here
> case board_20319:
> probe_ent->n_ports = 4;
>
> @@ -699,6 +757,9 @@ static int pdc_ata_init_one (struct pci_
> probe_ent->port[2].scr_addr = base + 0x600;
> probe_ent->port[3].scr_addr = base + 0x700;
> break;
> + case board_2057x:
> + /* Override hotplug offset for SATAII150 */
> + hp->hotplug_offset = PDC2_SATA_PLUG_CSR;
ditto
> case board_2037x:
> probe_ent->n_ports = 2;
> break;
> @@ -724,7 +785,7 @@ static int pdc_ata_init_one (struct pci_
> /* initialize adapter */
> pdc_host_init(board_idx, probe_ent);
>
> - /* 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.
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
Jeff
On 9/28/05, Jeff Garzik <[email protected]> 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
Lukasz Kosewski wrote:
> No. ata_device_add returns nonzero on success; so say the docs.
Whoops, you're right. I forget it was special.
> 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.
It still needs to clean up after pdc_host_init()...
Jeff