On 10/11/2010 02:55 PM, Yang, Bo wrote: > Tomas, > > >> Another correction - flush_scheduled_work is already present in megass_detach_one >> it only should be moved away from the if statement. >> > The flush_scheduled_work is for schedule_delayed_work(). We need to flush it and remove. > I'm not saying it should be removed, I think a move outside from the 'if' block makes it work for ev->hotplug_work and for the newly added instance->work_init diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c index 55951f4..7773707 100644 --- a/drivers/scsi/megaraid/megaraid_sas.c +++ b/drivers/scsi/megaraid/megaraid_sas.c @@ -4088,9 +4088,9 @@ static void __devexit megasas_detach_one(struct pci_dev *pdev) struct megasas_aen_event *ev = instance->ev; cancel_delayed_work( (struct delayed_work *)&ev->hotplug_work); - flush_scheduled_work(); - instance->ev = NULL; } + flush_scheduled_work(); + instance->ev = NULL; tasklet_kill(&instance->isr_tasklet); -- > -----Original Message----- > From: Tomas Henzl [mailto:thenzl@xxxxxxxxxx] > Sent: Saturday, October 09, 2010 4:38 PM > To: James Bottomley > Cc: bo yang; linux-scsi@xxxxxxxxxxxxxxx; akpm@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Yang, Bo > Subject: Re: PATCH 1/5] scsi: megaraid_sas - Add Online Controller Reset to MegaRAID SAS drive > > On 10/08/2010 09:28 PM, Tomas Henzl wrote: > >> On 10/08/2010 06:39 PM, James Bottomley wrote: >> >> >>> On Fri, 2010-10-08 at 17:51 +0200, Tomas Henzl wrote: >>> >>> >>> >>>> On 09/23/2010 04:36 AM, bo yang wrote: >>>> >>>> >>>> >>>>> This patch is too big. I am using attachment to submit. Please >>>>> use attached file to apply. Also let me know if it can't be accepted. >>>>> >>>>> To add the Online controller reset support, driver need to do: >>>>> a). reset the controller chips -- Xscale and Gen2 which will change >>>>> the function calls and add the reset function related to this two >>>>> chips. >>>>> b). during the reset, driver will store the pending cmds which not >>>>> returned by FW to driver's pending queue. Driver will re-issue those >>>>> pending cmds again to FW after the OCR finished. >>>>> c). In driver's timeout routine, driver will report to OS as reset. >>>>> Also driver's queue routine will block the cmds until the OCR >>>>> finished. >>>>> d). in Driver's ISR routine, if driver get the FW state as state >>>>> change, FW in Failure status and FW support online controller >>>>> reset (OCR), driver will start to do the controller reset. >>>>> e). In driver's IOCTL routine, the application cmds will wait for the >>>>> OCR to finish, then issue the cmds to FW. >>>>> >>>>> Signed-off-by Bo Yang<bo.yang@xxxxxxx> >>>>> >>>>> --- >>>>> drivers/scsi/megaraid/megaraid_sas.c | 756 ++++++++++++++++++++++++++++++++--- >>>>> drivers/scsi/megaraid/megaraid_sas.h | 88 +++- >>>>> 2 files changed, 787 insertions(+), 57 deletions(-) >>>>> >>>>> >>>>> >>>> Hi Bo, >>>> in the workqueue function you sleep for 30s, >>>> it's scheduled here - schedule_work(&instance->work_init); >>>> >>>> +process_fw_state_change_wq(struct work_struct *work) >>>> +{ >>>> ... >>>> + /*waitting for about 20 second before start the second init*/ >>>> + for (wait = 0; wait < 30; wait++) { >>>> + msleep(1000); >>>> + } >>>> >>>> >>>> >>> this lot should be ssleep(20) if you want a 20 sec sleep. >>> >>> >>> >> please do that on every place where you use the >> "for (wait = 0; wait < n; wait++) msleep(1000);" construction >> >> >> >>>> - this is not a good practice to sleep for a so long time I think >>>> >>>> >>>> >> this long sleep might might be ok, if the workqueue is used only rarely >> is it so? >> >> >> >>>> - you should use in your exit function some synchronization >>>> for example 'cancel_work_sync', without that if someone rmmods your >>>> module, it could then lead to a memory corruption >>>> >>>> >>>> >>> Actually flush_scheduled_work() should be fine ... it will force the >>> module removal to wait for completion ... cancellation can be error >>> prone, so just forcing the wait sounds easier. >>> >>> >>> > Another correction - flush_scheduled_work is already present in megass_detach_one > it only should be moved away from the if statement. > > > >> someone told that cancel_work_sync is safer then flush_scheduled_work >> but I'm not an expert, so ok >> >> Tomas >> >> >> >>> James >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > NïïïïïrïïyïïïbïXïïÇvï^ï)Þ{.nï+ïïïï{ïïï"ï{ayï ÊÚï,jïïfïïïhïïïzï ïwïïï ïïïj:+vïïïwïjïmïïïïïïïïzZ+ïïÝj"ïï!tml= -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
