On 7/27/19 5:37 AM, Douglas Gilbert wrote:
> Introduce bitops in sg_device to replace an atomic, a bool and a
> char. That char (sgdebug) had been reduced to only two states.
> Add some associated macros to make the code a little clearer.
>
> Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
> ---
> drivers/scsi/sg.c | 104 +++++++++++++++++++++++-----------------------
> 1 file changed, 53 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 9aa1b1030033..97ce84f0c51b 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -74,6 +74,11 @@ static char *sg_version_date = "20190606";
>
> #define SG_DEFAULT_TIMEOUT mult_frac(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
>
> +/* Bit positions (flags) for sg_device::fdev_bm bitmask follow */
> +#define SG_FDEV_EXCLUDE 0 /* have fd open with O_EXCL */
> +#define SG_FDEV_DETACHING 1 /* may be unexpected device removal */
> +#define SG_FDEV_LOG_SENSE 2 /* set by ioctl(SG_SET_DEBUG) */
> +
> int sg_big_buff = SG_DEF_RESERVED_SIZE;
> /* N.B. This variable is readable and writeable via
> /proc/scsi/sg/def_reserved_size . Each time sg_open() is called a buffer
> @@ -155,14 +160,12 @@ struct sg_device { /* holds the state of each scsi generic device */
> struct scsi_device *device;
> wait_queue_head_t open_wait; /* queue open() when O_EXCL present */
> struct mutex open_rel_lock; /* held when in open() or release() */
> - int sg_tablesize; /* adapter's max scatter-gather table size */
> - u32 index; /* device index number */
> struct list_head sfds;
> rwlock_t sfd_lock; /* protect access to sfd list */
> - atomic_t detaching; /* 0->device usable, 1->device detaching */
> - bool exclude; /* 1->open(O_EXCL) succeeded and is active */
> + int sg_tablesize; /* adapter's max scatter-gather table size */
> + u32 index; /* device index number */
> int open_cnt; /* count of opens (perhaps < num(sfds) ) */
> - char sgdebug; /* 0->off, 1->sense, 9->dump dev, 10-> all devs */
> + unsigned long fdev_bm[1]; /* see SG_FDEV_* defines above */
Just use 'unsigned long fdev_bm' (or maybe 'unsigned long fdev_flags').
No point in declaring a one-entry array.
> struct gendisk *disk;
> struct cdev * cdev; /* char_dev [sysfs: /sys/cdev/major/sg<n>] */
> struct kref d_ref;
> @@ -200,6 +203,9 @@ static void sg_device_destroy(struct kref *kref);
> #define SZ_SG_IO_HDR ((int)sizeof(struct sg_io_hdr)) /* v3 header */
> #define SZ_SG_REQ_INFO ((int)sizeof(struct sg_req_info))
>
> +#define SG_IS_DETACHING(sdp) test_bit(SG_FDEV_DETACHING, (sdp)->fdev_bm)
> +#define SG_HAVE_EXCLUDE(sdp) test_bit(SG_FDEV_EXCLUDE, (sdp)->fdev_bm)
> +
> /*
> * Kernel needs to be built with CONFIG_SCSI_LOGGING to see log messages.
> * 'depth' is a number between 1 (most severe) and 7 (most noisy, most
> @@ -273,26 +279,26 @@ sg_wait_open_event(struct sg_device *sdp, bool o_excl)
> while (sdp->open_cnt > 0) {
> mutex_unlock(&sdp->open_rel_lock);
> retval = wait_event_interruptible(sdp->open_wait,
> - (atomic_read(&sdp->detaching) ||
> + (SG_IS_DETACHING(sdp) ||
> !sdp->open_cnt));
> mutex_lock(&sdp->open_rel_lock);
>
> if (retval) /* -ERESTARTSYS */
> return retval;
> - if (atomic_read(&sdp->detaching))
> + if (SG_IS_DETACHING(sdp))
> return -ENODEV;
> }
> } else {
> - while (sdp->exclude) {
> + while (SG_HAVE_EXCLUDE(sdp)) {
> mutex_unlock(&sdp->open_rel_lock);
> retval = wait_event_interruptible(sdp->open_wait,
> - (atomic_read(&sdp->detaching) ||
> - !sdp->exclude));
> + (SG_IS_DETACHING(sdp) ||
> + !SG_HAVE_EXCLUDE(sdp)));
> mutex_lock(&sdp->open_rel_lock);
>
> if (retval) /* -ERESTARTSYS */
> return retval;
> - if (atomic_read(&sdp->detaching))
> + if (SG_IS_DETACHING(sdp))
> return -ENODEV;
> }
> }
> @@ -354,7 +360,7 @@ sg_open(struct inode *inode, struct file *filp)
> goto error_mutex_locked;
> }
> } else {
> - if (sdp->exclude) {
> + if (SG_HAVE_EXCLUDE(sdp)) {
> retval = -EBUSY;
> goto error_mutex_locked;
> }
> @@ -367,10 +373,10 @@ sg_open(struct inode *inode, struct file *filp)
>
> /* N.B. at this point we are holding the open_rel_lock */
> if (o_excl)
> - sdp->exclude = true;
> + set_bit(SG_FDEV_EXCLUDE, sdp->fdev_bm);
>
> if (sdp->open_cnt < 1) { /* no existing opens */
> - sdp->sgdebug = 0;
> + clear_bit(SG_FDEV_LOG_SENSE, sdp->fdev_bm);
> q = sdp->device->request_queue;
> sdp->sg_tablesize = queue_max_segments(q);
> }
Do not use 'set_bit' and 'clear_bit' on their own; the do not imply any
memory barriers, so concurrent open() calls will not necessarily see
the real value.
So either use some XX_bit() variant which returns a value (like
test_and_set_bit()), or add an explicit memory barrier.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
