Re: [PATCH v3 3/6] [SCSI] Add support for scsi_target events

[Bart Van Assche <bvanassche@acm.org>]

On 06/19/13 19:42, Ewan D. Milne wrote:
> +static void starget_evt_emit(struct scsi_target *starget,
> +			     struct starget_event *evt)
> +{
> +	int idx = 0;
> +	char *envp[3];
> +
> +	switch (evt->evt_type) {
> +	case STARGET_EVT_LUN_CHANGE_REPORTED:
> +		envp[idx++] = "STARGET_LUN_CHANGE_REPORTED=1";
> +		break;
> +	default:
> +		/* do nothing */
> +		break;
> +	}
> +
> +	envp[idx++] = NULL;
> +
> +	kobject_uevent_env(&starget->dev.kobj, KOBJ_CHANGE, envp);
> +}

Sorry but it's not clear to me why the envp[] array has size three while 
at most two entries are used ? And shouldn't that array be declared as 
const char*[] instead of char *[] since string constants have type const 
char[] ?

> +		list_for_each_safe(this, tmp, &event_list) {
> +			evt = list_entry(this, struct starget_event, node);

Any reason why list_for_each_entry_safe() has not been used here ?

> +void starget_evt_send(struct scsi_target *starget, struct starget_event *evt)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&starget->list_lock, flags);
> +	list_add_tail(&evt->node, &starget->event_list);
> +	schedule_work(&starget->event_work);
> +	spin_unlock_irqrestore(&starget->list_lock, flags);
> +}

Is it necessary here to invoke schedule_work() under protection of 
list_lock, or would it be safe to invoke schedule_work() after the 
spin_unlock_irqrestore() ?

> +#ifdef CONFIG_SCSI_ENHANCED_UA
> +	struct list_head *this, *tmp;
> +
> +	cancel_work_sync(&starget->event_work);
> +
> +	list_for_each_safe(this, tmp, &starget->event_list) {
> +		struct starget_event *evt;
>
> +		evt = list_entry(this, struct starget_event, node);
> +		list_del(&evt->node);
> +		kfree(evt);
> +	}
> +#endif

Same question here: any reason why list_for_each_entry_safe() has not 
been used ?

Thanks,

Bart.