Re: [PATCH] scsi: make 'state' device attribute pollable

[Christoph Hellwig <hch@lst.de>]

On Wed, Aug 09, 2017 at 03:09:18PM +0200, Hannes Reinecke wrote:
> While the 'state' attribute can (and will) change occasionally,
> calling 'poll()' or 'select()' on it fails as sysfs is never
> notified that the state has changed.
> With this patch calling 'poll()' or 'select()' will work
> properly.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_lib.c           | 7 +++++--
>  drivers/scsi/scsi_transport_srp.c | 5 ++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 41c19c7..2101cfd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2654,6 +2654,7 @@ void scsi_exit_queue(void)
>  
>  	}
>  	sdev->sdev_state = state;
> +	sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
>  	return 0;
>  
>   illegal:
> @@ -3074,14 +3075,16 @@ int scsi_internal_device_unblock_nowait(struct scsi_device *sdev,
>  	 * offlined states and goose the device queue if successful.
>  	 */
>  	if ((sdev->sdev_state == SDEV_BLOCK) ||
> -	    (sdev->sdev_state == SDEV_TRANSPORT_OFFLINE))
> +	    (sdev->sdev_state == SDEV_TRANSPORT_OFFLINE)) {
>  		sdev->sdev_state = new_state;
> -	else if (sdev->sdev_state == SDEV_CREATED_BLOCK) {
> +		sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
> +	} else if (sdev->sdev_state == SDEV_CREATED_BLOCK) {
>  		if (new_state == SDEV_TRANSPORT_OFFLINE ||
>  		    new_state == SDEV_OFFLINE)
>  			sdev->sdev_state = new_state;
>  		else
>  			sdev->sdev_state = SDEV_CREATED;
> +		sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
>  	} else if (sdev->sdev_state != SDEV_CANCEL &&
>  		 sdev->sdev_state != SDEV_OFFLINE)
>  		return -EINVAL;

This would be a lot more readable using switch statements:

	switch (sdev->sdev_state) {
	case SDEV_BLOCK:
	case SDEV_TRANSPORT_OFFLINE:
		sdev->sdev_state = new_state;
		sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
		break;
	case SDEV_CREATED_BLOCK:
  		switch (new_state) {
		case SDEV_TRANSPORT_OFFLINE:
 		case SDEV_OFFLINE:
  			sdev->sdev_state = new_state;
			break;
		default:
  			sdev->sdev_state = SDEV_CREATED;
			break;
		}
		sysfs_notify(&sdev->sdev_gendev.kobj, NULL, "state");
		break;
	case SDEV_CANCEL:
	case SDEV_OFFLINE:
		break;
	default:
 		return -EINVAL;
	}

Otherwise it looks fine to me.