get rid of ->finish method for highlevel drivers
PATCH 1/1 get rid of ->finish method for highlevel drivers
the ->finish method is a relicat from the old day were we never had hotplugging and allowed the driver to do fixups after all busses had been scanned. Nowdays only sd and sr actually implement it, and both only defer actions to there that should actually happen in ->attach. Change both drivers to move that code into ->attach, clenaup the Templates to use C99 initializers and get rid of the methods.
This also cleans up some very crude race-avoidable code in those drivers, btw..
drivers/scsi/hosts.c +0 -8
@@ -573,14 +573,6 @@ int scsi_register_host(Scsi_Host_Templat } } } - -/* This does any final handling that is required. */ -for (sdev_tp = scsi_devicelist; sdev_tp; - sdev_tp = sdev_tp->next) { -if (sdev_tp->finish && sdev_tp->nr_dev) { - (*sdev_tp->finish) (); - } - } }return 0 ;
drivers/scsi/hosts.h +0 -1
@@ -583,7 +583,6 @@ struct Scsi_Device_Templateint (*detect)(Scsi_Device *);/* Returns 1 if we can attach this device */ int (*init)(void );/* Sizes arrays based upon number of devices * detected */ -void (*finish)(void );/* Perform initialization after attachment */ int (*attach)(Scsi_Device *);/* Attach devices to arrays */ void (*detach)(Scsi_Device *);int (*init_command)(Scsi_Cmnd *);/* Used by new queueing code.
drivers/scsi/scsi.c +3 -7
@@ -2034,18 +2039,14 @@ int scsi_register_device(struct Scsi_Dev } } -/* -* This does any final handling that is required. -*/ -if (tpnt->finish && tpnt->nr_dev) - (*tpnt->finish) ();MOD_INC_USE_COUNT ;if (out_of_space) {scsi_unregister_device (tpnt);/* easiest way to clean up?? */ return 1 ; - }else -return 0 ; + } + +return 0 ; }int scsi_unregister_device (struct Scsi_Device_Template *tpnt)
drivers/scsi/scsi_scan.c +0 -5
@@ -2012,11 +2012,6 @@ static void scsi_scan_selected_lun(struc __FUNCTION__); } } - -for (sdt = scsi_devicelist; sdt; sdt = sdt->next) -if (sdt->finish && sdt->nr_dev) - (*sdt->finish) (); - } }
drivers/scsi/sd.c +42 -70
177 lines not on this page · raw message
drivers/scsi/sd.h +0 -1
@@ -25,7 +25,6 @@ typedef struct scsi_disk {Scsi_Device *device;unsigned char media_present;unsigned char write_prot; - unsigned has_been_registered:1; unsigned WCE:1; /* state of disk WCE bit */ unsigned RCD:1; /* state of disk RCD bit */ } Scsi_Disk;
drivers/scsi/sr.c +67 -72
186 lines not on this page · raw message
Quoted 6 lines · [email protected] said:
the ->finish method is a relicat from the old day were we never had hotplugging and allowed the driver to do fixups after all busses had been scanned. Nowdays only sd and sr actually implement it, and both only defer actions to there that should actually happen in ->attach. Change both drivers to move that code into ->attach, clenaup the Templates to use C99 initializers and get rid of the methods.
OK, this one causes a hang on boot for me. I'll look at debugging the cause some more tomorrow.
James
Quoted 6 lines · [email protected] said:
the ->finish method is a relicat from the old day were we never had hotplugging and allowed the driver to do fixups after all busses had been scanned. Nowdays only sd and sr actually implement it, and both only defer actions to there that should actually happen in ->attach. Change both drivers to move that code into ->attach, clenaup the Templates to use C99 initializers and get rid of the methods.
Quoted 2 lines · [email protected] said:
OK, this one causes a hang on boot for me. I'll look at debugging the cause some more tomorrow.
I tracked down the essential problem: the CD rom finish method expects to have allocated command blocks to do a mode sense. I also fixed a trivial initialisation problem (the device pointer in Scsi_CD was being used before it was initialised).
The attached patch fixes the problem for me.
I still can't detach and add a CD-ROM, but the panic is in sr_init, so doesn't look to be affected by these changes, but I'll continue debugging.
James
===== drivers/scsi/hosts.c 1.17 vs edited =====
drivers/scsi/hosts.c +5 -4
@@ -561,15 +561,16 @@ shost = list_entry(lh, struct Scsi_Host, sh_list);for (sdev = shost->host_queue; sdev; sdev = sdev->next)if (sdev->host->hostt == shost_tp) { +scsi_build_commandblocks (sdev); +if (sdev->current_queue_depth ==0 ) + goto out_of_space;for (sdev_tp = scsi_devicelist; sdev_tp; sdev_tp = sdev_tp->next)if (sdev_tp->attach) (*sdev_tp->attach) (sdev); -if (sdev->attached) { -scsi_build_commandblocks (sdev); -if (sdev->current_queue_depth ==0 ) - goto out_of_space; +if (!sdev->attached) { +scsi_release_commandblocks (sdev); } } }
===== drivers/scsi/scsi.c 1.50 vs edited =====
drivers/scsi/scsi.c +9 -5
@@ -2024,18 +2024,22 @@ shpnt = scsi_host_get_next(shpnt)) {for (SDpnt = shpnt->host_queue; SDpnt; SDpnt = SDpnt->next) { +scsi_build_commandblocks (SDpnt); +if (SDpnt->current_queue_depth ==0 ) { + out_of_space =1 ; +continue ; + }if (tpnt->attach) (*tpnt->attach) (SDpnt); +/* * If this driver attached to the device, and don't have any * command blocks for this device, allocate some. */ -if (SDpnt->attached && SDpnt->current_queue_depth ==0 ) { +if (SDpnt->attached) SDpnt->online = TRUE; -scsi_build_commandblocks (SDpnt); -if (SDpnt->current_queue_depth ==0 ) - out_of_space =1 ; - } +else +scsi_release_commandblocks (SDpnt); } }
===== drivers/scsi/scsi_scan.c 1.29 vs edited =====
drivers/scsi/scsi_scan.c +20 -16
@@ -1995,24 +1995,28 @@ sdevscan->scsi_level =scsi_find_scsi_level (channel, id, shost); res =scsi_probe_and_add_lun (sdevscan, &sdev,NULL );scsi_free_sdev (sdevscan); -if (res ==SCSI_SCAN_LUN_PRESENT ) { -BUG_ON (sdev ==NULL ); -for (sdt = scsi_devicelist; sdt; sdt = sdt->next) -if (sdt->init && sdt->dev_noticed) - (*sdt->init) (); - -for (sdt = scsi_devicelist; sdt; sdt = sdt->next) -if (sdt->attach) { - (*sdt->attach) (sdev); -if (sdev->attached) { -scsi_build_commandblocks (sdev); -if (sdev->current_queue_depth ==0 ) -printk (ALLOC_FAILURE_MSG , - __FUNCTION__); - } - } +if (res !=SCSI_SCAN_LUN_PRESENT ) +return ; + +BUG_ON (sdev ==NULL ); + +scsi_build_commandblocks (sdev); +if (sdev->current_queue_depth ==0 ) { +printk (ALLOC_FAILURE_MSG , __FUNCTION__); +return ; } + +for (sdt = scsi_devicelist; sdt; sdt = sdt->next) +if (sdt->init && sdt->dev_noticed) + (*sdt->init) (); + +for (sdt = scsi_devicelist; sdt; sdt = sdt->next) +if (sdt->attach) + (*sdt->attach) (sdev); + +if (!sdev->attached) +scsi_release_commandblocks (sdev); }/**
===== drivers/scsi/sr.c 1.58 vs edited =====
drivers/scsi/sr.c +2 -2
@@ -481,10 +481,10 @@if (i >= sr_template.dev_max)panic ("scsi_devices corrupt (sr)" ); + scsi_CDs[i].device = SDp; +if (sr_init_one (cpnt, i)) goto fail; - - scsi_CDs[i].device = SDp; sr_template.nr_dev++;if (sr_template.nr_dev > sr_template.dev_max)
Quoted 6 lines · James Bottomley
I tracked down the essential problem: the CD rom finish method expects to have allocated command blocks to do a mode sense. I also fixed a trivial initialisation problem (the device pointer in
Scsi_CDwas being used before it was initialised).
Quoted 7 lines
The attached patch fixes the problem for me.
I still can't detach and add a CD-ROM, but the panic is in
sr_init, so doesn't look to be affected by these changes, but I'll continue debugging.James
James -
How do we end up with no command blocks if the slave_attach is calling scsi_build_commandblocks via scsi_adjust_queue_depth?
-- Patrick Mansfield
Quoted 12 lines · Patrick Mansfield [[email protected]] wrote:
Quoted 11 lines · James Bottomley
I tracked down the essential problem: the CD rom finish method expects to have allocated command blocks to do a mode sense. I also fixed a trivial initialisation problem (the device pointer in
Scsi_CDwas being used before it was initialised).The attached patch fixes the problem for me.
I still can't detach and add a CD-ROM, but the panic is in
sr_init, so doesn't look to be affected by these changes, but I'll continue debugging.
Quoted 7 lines
Quoted 2 lines
James
James -
How do we end up with no command blocks if the
slave_attachis callingscsi_build_commandblocksviascsi_adjust_queue_depth?
It appears after looking at this with Patrick that in drivers where slave_attach is is not calling scsi_adjust_queue_depth the scsi_build_commandblocks will not be called. In my case ips.c and in James case 53c700.c.
If post calling slave_attach the new_queue_depth is 0 and the device is not offline we should ensure there is at least a new_queue_depth of 1 which is what your patch does by later on calling scsi_build_commandblocks directly.
-andmike
--
Michael Anderson
[email protected]
Quoted 26 lines · Mike Anderson
Quoted 12 lines · Patrick Mansfield [[email protected]] wrote:
Quoted 11 lines · James Bottomley
I tracked down the essential problem: the CD rom finish method expects to have allocated command blocks to do a mode sense. I also fixed a trivial initialisation problem (the device pointer in
Scsi_CDwas being used before it was initialised).The attached patch fixes the problem for me.
I still can't detach and add a CD-ROM, but the panic is in
sr_init, so doesn't look to be affected by these changes, but I'll continue debugging.Quoted 7 lines
Quoted 2 lines
James
James -
How do we end up with no command blocks if the
slave_attachis callingscsi_build_commandblocksviascsi_adjust_queue_depth?It appears after looking at this with Patrick that in drivers where
slave_attachis is not callingscsi_adjust_queue_depththescsi_build_commandblockswill not be called. In my caseips.cand in James case53c700.c.
Quoted 4 lines
If post calling
slave_attachthenew_queue_depthis 0 and the device is not offline we should ensure there is at least anew_queue_depthof 1 which is what your patch does by later on callingscsi_build_commandblocksdirectly.
Actually, prior to the scsi_scan changes, the code always made sure that it was at least 1 after the scan. Maybe that got lost somehow...
Signature · 4 lines
Doug Ledford <[email protected]> 919-754-3700 x44233 Red Hat, Inc. 1801 Varsity Dr. Raleigh, NC 27606
Quoted 10 lines · Christoph Hellwig [[email protected]] wrote:
the ->finish method is a relicat from the old day were we never had hotplugging and allowed the driver to do fixups after all busses had been scanned. Nowdays only sd and sr actually implement it, and both only defer actions to there that should actually happen in ->attach. Change both drivers to move that code into ->attach, clenaup the Templates to use C99 initializers and get rid of the methods.
This also cleans up some very crude race-avoidable code in those drivers, btw..
Looks good Christoph. I applied this patch to James scsi-misc-2.5 tree and I ran it under UML and did not see any boot / shutdown problems. I will run it on our FC 2x tomorrow.
It looks to me like we could also investigate removing sd_disks and sd_dsk_arr by using the per driver storage of the device model. In the 2 sd_disks access cases we have the scsi_device and should be able to locate the device object. I believe the sd_dsk_arr could also be covered in a similar method.
-andmike
--
Michael Anderson
[email protected]
Quoted 5 lines · Mike Anderson
It looks to me like we could also investigate removing
sd_disksandsd_dsk_arrby using the per driver storage of the device model. In the 2sd_disksaccess cases we have thescsi_deviceand should be able to locate the device object. I believe thesd_dsk_arrcould also be covered in a similar method.
Yes, that's on my TODO list for this week.