RE: [PATCH 2/10] scsi: megaraid_sas - Add Poll Wait mechanism toMegaRAID SAS driver (PART-I)

[Yang, Bo]

Andrew,

Thanks for the review and good catch.  The following is the changes I did:

1. 

> +       if ((!cmd->abort_aen) && (instance->unload == 0)) {
> +               megasas_aen_polling(instance);
> +       }

> I see you're not a big fan of checkpatch, nor of kernel coding style :(

Changed to:
+       if (!cmd->abort_aen && instance->unload == 0) {
+               megasas_aen_polling(instance);
+       }

2.

> +       /*
> +       * cancel the delayed work if this work still in queue
> +       */
> +       if (instance->hotplug_wk != NULL) {
> +               cancel_delayed_work(
> +                       (struct delayed_work *)instance->hotplug_wk);

>whah?  Can't do that!  cancel_delayed_work() will do del_timer_sync()
>on megasas_instance.flag and will crash.

> +                       

> +               flush_scheduled_work();

Changed to:
+
+       /*
+       * cancel the delayed work if this work still in queue
+       */
+       if (instance->ev != NULL) {
+               struct megasas_aen_event *ev = instance->ev;
+               cancel_delayed_work(
+                       (struct delayed_work *)&ev->hotplug_work);
+               flush_scheduled_work();
+               instance->ev = NULL;
+       }
        tasklet_kill(&instance->isr_tasklet);

.....

+                       ev->instance = instance;
+                       instance->ev = ev;
+                       INIT_WORK(&ev->hotplug_work, megasas_scsi_scan_host);
+                       schedule_delayed_work(
+                               (struct delayed_work *)&ev->hotplug_work, 0);


3.
> +       instance->hotplug_wk = NULL;
> +       if (!instance) {

>oh come on.  If this test returns true then the preceding statement
>just oopsed the kernel.

> +               printk(KERN_ERR "%s: invalid instance!\n", __FUNCTION__); 

Changed to:

+static void
+megasas_scsi_scan_host(struct work_struct *work)
+{
+       struct megasas_aen_event *ev = container_of(work,
+               struct megasas_aen_event, hotplug_work);
+       struct megasas_instance *instance = ev->instance;
+
+       if (!instance) {
+               printk(KERN_ERR "invalid instance!\n");
+               kfree(ev);
+               return;
+       }
+
+       printk(KERN_INFO "scanning ...\n");
+       scsi_scan_host(instance->host);
+       kfree(ev);
+       instance->ev = NULL;
+}

4. 

> +       if (instance->evt_detail) {
> +               printk(KERN_INFO "%s[%d]: event code 0x%04x\n", __FUNCTION__,

Users of checkpatch know to use __func__.

> +                       instance->host->host_no, instance->evt_detail-

Changed to:

+               printk(KERN_INFO "get event code 0x%04x\n",
+                       instance->evt_detail->code); 


5.

> +
> +       /**
> +       * Register AEN with FW for latest sequence number plus 1
> +       **/

>The /** token is used to introduce a kerneldoc comment.  This is not a
>kerneldoc comment.

Changed to:

+	/* Register AEN with FW for latest sequence number plus 1 */

6. 
>         struct tasklet_struct isr_tasklet;
> +       struct work_struct *hotplug_wk;

>"hotplug_work"

+       struct megasas_aen_event *ev;

 
Thanks,

Bo Yang

-----Original Message-----
From: Andrew Morton [mailto:akpm@xxxxxxxxxxxxxxxxxxxx] 
Sent: Wednesday, May 06, 2009 5:17 PM
To: Yang, Bo
Cc: Yang, Bo; James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Austria, Winston
Subject: Re: [PATCH 2/10] scsi: megaraid_sas - Add Poll Wait mechanism to MegaRAID SAS driver (PART-I)

On Tue, 5 May 2009 18:25:55 -0600
"Yang, Bo" <Bo.Yang@xxxxxxx> wrote:

> Add Poll_wait mechanism to SAS-2 MegaRAID SAS Linux driver. Driver will wakeup poll after the driver get event from MegaRAID SAS FW.  Driver also reregisters for Event with the FW when it receives AEN.
> 

Seems fairly strage to be adding poll support to a scsi driver, but I
see it isn't the first driver to do this.

> 
>
> ...
>
> @@ -1295,13 +1301,20 @@ megasas_service_aen(struct megasas_insta
>         /*
>          * Don't signal app if it is just an aborted previously registered aen
>          */
> -       if (!cmd->abort_aen)
> +       if ((!cmd->abort_aen) && (instance->unload == 0)) {
> +               megasas_poll_wait_aen = 1;
> +               wake_up(&megasas_poll_wait);
>                 kill_fasync(&megasas_async_queue, SIGIO, POLL_IN);
> +       }
>         else
>                 cmd->abort_aen = 0;
> 
>         instance->aen_cmd = NULL;
>         megasas_return_cmd(instance, cmd);
> +
> +       if ((!cmd->abort_aen) && (instance->unload == 0)) {
> +               megasas_aen_polling(instance);
> +       }

I see you're not a big fan of checkpatch, nor of kernel coding style :(

>  }
> 
>  /*
> @@ -2583,6 +2596,8 @@ megasas_probe_one(struct pci_dev *pdev,
> 
>         *instance->producer = 0;
>         *instance->consumer = 0;
> +       megasas_poll_wait_aen = 0;
> +       instance->hotplug_wk = NULL;
> 
>         instance->evt_detail = pci_alloc_consistent(pdev,
>                                                     sizeof(struct
> @@ -2621,6 +2636,7 @@ megasas_probe_one(struct pci_dev *pdev,
> 
>         megasas_dbg_lvl = 0;
>         instance->flag = 0;
> +       instance->unload = 0;
>         instance->last_time = 0;
> 
>         /*
> @@ -2924,6 +2940,7 @@ static void __devexit megasas_detach_one
>         struct megasas_instance *instance;
> 
>         instance = pci_get_drvdata(pdev);
> +       instance->unload = 1;
>         host = instance->host;
> 
>         if (poll_mode_io)
> @@ -2932,6 +2949,15 @@ static void __devexit megasas_detach_one
>         scsi_remove_host(instance->host);
>         megasas_flush_cache(instance);
>         megasas_shutdown_controller(instance, MR_DCMD_CTRL_SHUTDOWN);
> +
> +       /*
> +       * cancel the delayed work if this work still in queue
> +       */
> +       if (instance->hotplug_wk != NULL) {
> +               cancel_delayed_work(
> +                       (struct delayed_work *)instance->hotplug_wk);

whah?  Can't do that!  cancel_delayed_work() will do del_timer_sync()
on megasas_instance.flag and will crash.

> +                       

> +               flush_scheduled_work();
> +       }
>         tasklet_kill(&instance->isr_tasklet);
> 
>         /*
> @@ -3027,6 +3053,19 @@ static int megasas_mgmt_fasync(int fd, s
>  }
> 
>  /**
> + * megasas_mgmt_poll -  char node "poll" entry point
> + * */
> +static unsigned int megasas_mgmt_poll(struct file *file, poll_table *wait)
> +{
> +       unsigned int mask = 0;
> +       poll_wait(file, &megasas_poll_wait, wait);
> +       if (megasas_poll_wait_aen) {
> +               mask |=   (POLLIN | POLLRDNORM);
> +       }
> +       return mask;
> +}
> +
> +/**
>   * megasas_mgmt_fw_ioctl -     Issues management ioctls to FW
>   * @instance:                  Adapter soft state
>   * @argp:                      User's ioctl packet
> @@ -3348,6 +3387,7 @@ static const struct file_operations mega
>         .open = megasas_mgmt_open,
>         .fasync = megasas_mgmt_fasync,
>         .unlocked_ioctl = megasas_mgmt_ioctl,
> +       .poll = megasas_mgmt_poll,
>  #ifdef CONFIG_COMPAT
>         .compat_ioctl = megasas_mgmt_compat_ioctl,
>  #endif
> @@ -3462,6 +3502,100 @@ out:
>         return retval;
>  }
> 
> +static void
> +megasas_scsi_scan_host(struct work_struct *work)
> +{
> +       struct megasas_aen_event *ev = container_of(work,
> +               struct megasas_aen_event, hotplug_work);
> +       struct megasas_instance *instance = ev->instance;
> +
> +       instance->hotplug_wk = NULL;
> +       if (!instance) {

oh come on.  If this test returns true then the preceding statement
just oopsed the kernel.

> +               printk(KERN_ERR "%s: invalid instance!\n", __FUNCTION__);
> +               kfree(ev);
> +               return;
> +       }
> +
> +       printk(KERN_INFO "%s[%d]: scanning ...\n",
> +               __FUNCTION__, instance->host->host_no);
> +       scsi_scan_host(instance->host);
> +       kfree(ev);
> +}
> +
> +static void
> +megasas_aen_polling(struct megasas_instance *instance)
> +{
> +       union megasas_evt_class_locale class_locale;
> +       int doscan = 0;
> +       u32 seq_num;
> +       int error;
> +
> +       if (instance->evt_detail) {
> +               printk(KERN_INFO "%s[%d]: event code 0x%04x\n", __FUNCTION__,

Users of checkpatch know to use __func__.

> +                       instance->host->host_no, instance->evt_detail->code);
> +
> +               switch (instance->evt_detail->code) {
> +
> +               case MR_EVT_LD_CREATED:
> +               case MR_EVT_PD_INSERTED:
> +               case MR_EVT_LD_DELETED:
> +               case MR_EVT_LD_OFFLINE:
> +               case MR_EVT_CFG_CLEARED:
> +               case MR_EVT_PD_REMOVED:
> +               case MR_EVT_FOREIGN_CFG_IMPORTED:
> +               case MR_EVT_LD_STATE_CHANGE:
> +               case MR_EVT_CTRL_HOST_BUS_SCAN_REQUESTED:
> +                       doscan = 1;
> +                       break;
> +               default:
> +                       doscan = 0;
> +                       break;
> +               }
> +       } else {
> +               printk(KERN_ERR "%s[%d]: invalid evt_detail!\n",
> +                       __FUNCTION__, instance->host->host_no);
> +               return;
> +       }
> +
> +       if (doscan) {
> +               struct megasas_aen_event *ev;
> +               ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
> +               if (!ev) {
> +                       printk(KERN_ERR "%s: out of memory\n", __FUNCTION__);
> +               } else {
> +                       ev->instance = instance;
> +                       instance->hotplug_wk = &ev->hotplug_work;
> +                       INIT_WORK(&ev->hotplug_work, megasas_scsi_scan_host);
> +                       schedule_delayed_work(
> +                               (struct delayed_work *)&ev->hotplug_work, 0);
> +               }
> +       }
> +
> +       seq_num = instance->evt_detail->seq_num + 1;
> +
> +       /**
> +       * Register AEN with FW for latest sequence number plus 1
> +       **/

The /** token is used to introduce a kerneldoc comment.  This is not a
kerneldoc comment.

> +       class_locale.members.reserved = 0;
> +       class_locale.members.locale = MR_EVT_LOCALE_ALL;
> +       class_locale.members.class = MR_EVT_CLASS_DEBUG;
> +
> +       if ( instance->aen_cmd != NULL ) {
> +               return ;
> +       }
> +
> +       mutex_lock(&instance->aen_mutex);
> +       error = megasas_register_aen(instance, seq_num,
> +                               class_locale.word);
> +       mutex_unlock(&instance->aen_mutex);
> +
> +       if (error)
> +               printk(KERN_ERR "%s[%d]: register aen failed error %x\n",
> +                        __FUNCTION__, instance->host->host_no, error);
> +}
> +
> +
>
> ...
>
> @@ -1118,8 +1134,10 @@ struct megasas_instance {
> 
>         struct megasas_instance_template *instancet;
>         struct tasklet_struct isr_tasklet;
> +       struct work_struct *hotplug_wk;

"hotplug_work"

> 
>         u8 flag;
> +       u8 unload;
>         unsigned long last_time;
> 
>         struct timer_list io_completion_timer;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/