Re: [PATCH 1/1] [SCSI] gdth: misc cleanups, preparation for ISA/EISA hotplug API
On Sun, Feb 24, 2008 at 12:31:17AM -0500, Christoph Hellwig wrote: > On Sun, Feb 24, 2008 at 12:18:23AM -0500, Jeff Garzik wrote: > > hm. We'll see how it plays out... on the remove side, the above is > > exact what happens in gdth_remove_one() without my patch, thus > > consolidating two cases of the same code into one. There is a less-strong > > argument for doing the allocation that way, but it may turn out to be > > useful anyway once the ISA/EISA API conversion is complete. > > EISA ->remove has a different prototype from PCI ->remove from ISA > ->remove, so gdth_remove_one will be split up eventually. Having the > scsi_host_put duplicated in each shouldn't be too much of a problem :) Shouldn't need to duplicate it ... free bus-specific things in the ->remove method, and call a common helper. See advansys_release() and its callers in advansys.c for how I did it. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] [SCSI] gdth: misc cleanups, preparation for ISA/EISA hotplug API
Christoph Hellwig wrote: On Sun, Feb 24, 2008 at 12:18:23AM -0500, Jeff Garzik wrote: hm. We'll see how it plays out... on the remove side, the above is exact what happens in gdth_remove_one() without my patch, thus consolidating two cases of the same code into one. There is a less-strong argument for doing the allocation that way, but it may turn out to be useful anyway once the ISA/EISA API conversion is complete. EISA ->remove has a different prototype from PCI ->remove from ISA ->remove, so gdth_remove_one will be split up eventually. Having the scsi_host_put duplicated in each shouldn't be too much of a problem :) We'll see what the final result is... you might turn out to be right. If people want to avoid merging this patch because of this issue, that's fine. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] [SCSI] gdth: misc cleanups, preparation for ISA/EISA hotplug API
On Sun, Feb 24, 2008 at 12:18:23AM -0500, Jeff Garzik wrote: > hm. We'll see how it plays out... on the remove side, the above is > exact what happens in gdth_remove_one() without my patch, thus > consolidating two cases of the same code into one. There is a less-strong > argument for doing the allocation that way, but it may turn out to be > useful anyway once the ISA/EISA API conversion is complete. EISA ->remove has a different prototype from PCI ->remove from ISA ->remove, so gdth_remove_one will be split up eventually. Having the scsi_host_put duplicated in each shouldn't be too much of a problem :) - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] [SCSI] gdth: misc cleanups, preparation for ISA/EISA hotplug API
Christoph Hellwig wrote:
Eventually we shoud just kill the INT_COAL ifdefed code. It has never
been enabled and clutters up the driver quite badly.
Noted (queued)... fine by me, and makes life easier.
+#ifdef CONFIG_EISA
+ if ((ha->type == GDT_EISA) && (ha->ccb_phys))
+ pci_unmap_single(ha->pdev, ha->ccb_phys, sizeof(gdth_cmd_str),
+PCI_DMA_BIDIRECTIONAL);
+#endif /* CONFIG_EISA */
I don't think moving this into the common helper makes any sense, as
it's only ever done for the eisa adapter. Just keep it local there.
+#ifdef CONFIG_EISA
+ if (ha->type == GDT_EISA) {
+ ha->ccb_phys = pci_map_single(ha->pdev, &ha->cmdext,
+ sizeof(gdth_cmd_str), PCI_DMA_BIDIRECTIONAL);
+ if (!ha->ccb_phys)
+ goto out_free;
+ }
+#endif /* CONFIG_EISA */
Same here.
hm. We'll see how it plays out... on the remove side, the above is
exact what happens in gdth_remove_one() without my patch, thus
consolidating two cases of the same code into one. There is a
less-strong argument for doing the allocation that way, but it may turn
out to be useful anyway once the ISA/EISA API conversion is complete.
Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] [SCSI] gdth: misc cleanups, preparation for ISA/EISA hotplug API
On Sat, Feb 23, 2008 at 11:44:44PM -0500, Jeff Garzik wrote:
> Several misc. cleanups:
>
> - remove recently-noop'd 'reverse_scan' module parm
>
> - remove pointless function prototypes
>
> - remove ha->pccb, its value always == &ha->cmdext
>
> - move thrice-redundant DMA memory alloc and (in EISA's case, mapping)
> into common functions gdth_ha_alloc(), gdth_ha_free()
>
> - delete pointless zero-initializations of ha struct members, as these
> are zeroed when ha is allocated (and never assigned any other value,
> prior to the explicit zero initializations)
>
> - consolidate thrice-repeated spinlock init
Good idea!
> +static void gdth_ha_free(gdth_ha_str *ha)
> +{
> + if (ha->pscratch)
> + pci_free_consistent(ha->pdev, GDTH_SCRATCH,
> + ha->pscratch, ha->scratch_phys);
> + if (ha->pmsg)
> + pci_free_consistent(ha->pdev, sizeof(gdth_msg_str),
> + ha->pmsg, ha->msg_phys);
> +
> +#ifdef INT_COAL
> + if (ha->coal_stat)
> + pci_free_consistent(ha->pdev,
> + sizeof(gdth_coal_status) * MAXOFFSETS,
> + ha->coal_stat, ha->coal_stat_phys);
> +#endif
Eventually we shoud just kill the INT_COAL ifdefed code. It has never
been enabled and clutters up the driver quite badly.
> +#ifdef CONFIG_EISA
> + if ((ha->type == GDT_EISA) && (ha->ccb_phys))
> + pci_unmap_single(ha->pdev, ha->ccb_phys, sizeof(gdth_cmd_str),
> + PCI_DMA_BIDIRECTIONAL);
> +#endif /* CONFIG_EISA */
I don't think moving this into the common helper makes any sense, as
it's only ever done for the eisa adapter. Just keep it local there.
> +#ifdef CONFIG_EISA
> + if (ha->type == GDT_EISA) {
> + ha->ccb_phys = pci_map_single(ha->pdev, &ha->cmdext,
> + sizeof(gdth_cmd_str), PCI_DMA_BIDIRECTIONAL);
> + if (!ha->ccb_phys)
> + goto out_free;
> + }
> +#endif /* CONFIG_EISA */
Same here.
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] [SCSI] gdth: misc cleanups, preparation for ISA/EISA hotplug API
Several misc. cleanups:
- remove recently-noop'd 'reverse_scan' module parm
- remove pointless function prototypes
- remove ha->pccb, its value always == &ha->cmdext
- move thrice-redundant DMA memory alloc and (in EISA's case, mapping)
into common functions gdth_ha_alloc(), gdth_ha_free()
- delete pointless zero-initializations of ha struct members, as these
are zeroed when ha is allocated (and never assigned any other value,
prior to the explicit zero initializations)
- consolidate thrice-repeated spinlock init
Signed-off-by: Jeff Garzik <[EMAIL PROTECTED]>
---
NOTE: Applies on top of my previous two gdth patches (PCI hotplug prep,
PCI hotplug convert).
drivers/scsi/gdth.c | 299 ++
drivers/scsi/gdth.h |1 -
2 files changed, 108 insertions(+), 192 deletions(-)
diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index ad9aff2..b17eb15 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -51,8 +51,6 @@
* reserve_list:h,b,t,l,h,b,t,l,... reserve particular drive(s) with
* h- controller no., b- channel no.,
* t- target ID, l- LUN
- * reverse_scan:Y reverse scan order for PCI controllers
- * reverse_scan:N scan PCI controllers like BIOS
* max_ids:xx - target ID count per channel (1..MAXID)
* rescan:Y rescan all channels/IDs
* rescan:N use all devices found until now
@@ -66,7 +64,7 @@
* force_dma32:Yuse only 32 bit DMA mode
* force_dma32:Nuse 64 bit DMA mode, if supported
*
- * The default values are: "gdth=disable:N,reserve_mode:1,reverse_scan:N,
+ * The default values are: "gdth=disable:N,reserve_mode:1,
* max_ids:127,rescan:N,hdr_channel:0,
* shared_access:Y,probe_eisa_isa:N,force_dma32:N".
* Here is another example: "gdth=reserve_list:0,1,2,0,0,1,3,0,rescan:Y".
@@ -77,7 +75,7 @@
* with ' ' and all ':' with '=' and you must use
* '1' in place of 'Y' and '0' in place of 'N'.
*
- * Default: "modprobe gdth disable=0 reserve_mode=1 reverse_scan=0
+ * Default: "modprobe gdth disable=0 reserve_mode=1
* max_ids=127 rescan=0 hdr_channel=0 shared_access=0
* probe_eisa_isa=0 force_dma32=0"
* The other example: "modprobe gdth reserve_list=0,1,2,0,0,1,3,0 rescan=1".
@@ -148,29 +146,13 @@ static int gdth_sync_event(gdth_ha_str *ha, int service,
unchar index,
static int gdth_async_event(gdth_ha_str *ha);
static void gdth_log_event(gdth_evt_data *dvr, char *buffer);
-static void gdth_putq(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar priority);
-static void gdth_next(gdth_ha_str *ha);
static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b);
static int gdth_special_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp);
-static gdth_evt_str *gdth_store_event(gdth_ha_str *ha, ushort source,
- ushort idx, gdth_evt_data *evt);
static int gdth_read_event(gdth_ha_str *ha, int handle, gdth_evt_str *estr);
-static void gdth_readapp_event(gdth_ha_str *ha, unchar application,
- gdth_evt_str *estr);
-static void gdth_clear_events(void);
-static void gdth_copy_internal_data(gdth_ha_str *ha, Scsi_Cmnd *scp,
-char *buffer, ushort count, int to_buffer);
static int gdth_internal_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp);
static int gdth_fill_cache_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, ushort hdrive);
-static void gdth_enable_int(gdth_ha_str *ha);
-static int gdth_test_busy(gdth_ha_str *ha);
-static int gdth_get_cmd_index(gdth_ha_str *ha);
-static void gdth_release_event(gdth_ha_str *ha);
-static int gdth_wait(gdth_ha_str *ha, int index,ulong32 time);
-static int gdth_internal_cmd(gdth_ha_str *ha, unchar service, ushort opcode,
- ulong32 p1, ulong64 p2,ulong64
p3);
static int gdth_search_drives(gdth_ha_str *ha);
static int gdth_analyse_hdrive(gdth_ha_str *ha, ushort hdrive);
@@ -181,7 +163,6 @@ static int gdth_close(struct inode *inode, struct file
*filep);
static int gdth_ioctl(struct inode *inode, struct file *filep,
unsigned int cmd, unsigned long arg);
-static void gdth_flush(gdth_ha_str *ha);
static int gdth_halt(struct notifier_block *nb, ulong event, void *buf);
static int gdth_queuecommand(Scsi_Cmnd *scp,void (*done)(Scsi_Cmnd *));
static int __gdth_queuecommand(gdth_ha_str *ha, struct scsi_cmnd *scp,
@@ -336,8 +317,6 @@ static int reserve_list[MAX_RES_ARGS] =
{0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff};
-/* scan order for PCI controllers */
-static int reverse_scan = 0;
/* virtual channel for the host driv
