linux-kernel - Re: [PATCH 2/6] Refactor code to use new sas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 3 Jul 2015 08:38:03 -0700
From:	Christoph Hellwig <hch@...radead.org>
To:	Calvin Owens <calvinowens@...com>
Cc:	Nagalakshmi Nandigama <nagalakshmi.nandigama@...gotech.com>,
	Praveen Krishnamoorthy <praveen.krishnamoorthy@...gotech.com>,
	Sreekanth Reddy <sreekanth.reddy@...gotech.com>,
	Abhijit Mahajan <abhijit.mahajan@...gotech.com>,
	MPT-FusionLinux.pdl@...gotech.com, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH 2/6] Refactor code to use new sas_device refcount

>  
> +struct _sas_device *
> +mpt2sas_scsih_sas_device_get_by_sas_address_nolock(struct MPT2SAS_ADAPTER *ioc,
> +    u64 sas_address)

Any chance to use a shorter name for this function? E.g.
__mpt2sas_get_sdev_by_addr ?

> +{
> +	struct _sas_device *sas_device;
> +
> +	BUG_ON(!spin_is_locked(&ioc->sas_device_lock));

This will blow on UP builds.  Please use assert_spin_locked or
lockdep_assert_held instead.  And don't ask me which of the two,
that's a mystery I don't understand myself either.

>  struct _sas_device *
> -mpt2sas_scsih_sas_device_find_by_sas_address(struct MPT2SAS_ADAPTER *ioc,
> +mpt2sas_scsih_sas_device_get_by_sas_address(struct MPT2SAS_ADAPTER *ioc,
>      u64 sas_address)
>  {

> +static struct _sas_device *
> +_scsih_sas_device_get_by_handle_nolock(struct MPT2SAS_ADAPTER *ioc, u16 handle)

>  static struct _sas_device *
> -_scsih_sas_device_find_by_handle(struct MPT2SAS_ADAPTER *ioc, u16 handle)
> +_scsih_sas_device_get_by_handle(struct MPT2SAS_ADAPTER *ioc, u16 handle)

Same comments about the function names as above.

> +	struct _sas_device *sas_device;
> +
> +	BUG_ON(!spin_is_locked(&ioc->sas_device_lock));

Same comment about the right assert helpers as above.

> @@ -594,9 +634,15 @@ _scsih_sas_device_remove(struct MPT2SAS_ADAPTER *ioc,
>  	if (!sas_device)
>  		return;
>  
> +	/*
> +	 * The lock serializes access to the list, but we still need to verify
> +	 * that nobody removed the entry while we were waiting on the lock.
> +	 */
>  	spin_lock_irqsave(&ioc->sas_device_lock, flags);
> -	list_del(&sas_device->list);
> -	kfree(sas_device);
> +	if (!list_empty(&sas_device->list)) {
> +		list_del_init(&sas_device->list);
> +		sas_device_put(sas_device);
> +	}
>  	spin_unlock_irqrestore(&ioc->sas_device_lock, flags);

This looks odd to me.  Normally you'd have the lock from the list
iteration that finds the device.  From looking at the code it seems
like this only called from probe failure paths, though.  It seems like
for this case the device simplify shouldn't be added until the probe
succeeds and this function should go away?

> @@ -1208,12 +1256,15 @@ _scsih_change_queue_depth(struct scsi_device *sdev, int qdepth)
>  		goto not_sata;
>  	if ((sas_target_priv_data->flags & MPT_TARGET_FLAGS_VOLUME))
>  		goto not_sata;
> +
>  	spin_lock_irqsave(&ioc->sas_device_lock, flags);
> -	sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc,
> +	sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc,
>  	   sas_device_priv_data->sas_target->sas_address);
> -	if (sas_device && sas_device->device_info &
> -	    MPI2_SAS_DEVICE_INFO_SATA_DEVICE)
> +	if (sas_device && sas_device->device_info
> +			& MPI2_SAS_DEVICE_INFO_SATA_DEVICE) {
>  		max_depth = MPT2SAS_SATA_QUEUE_DEPTH;
> +		sas_device_put(sas_device);
> +	}

Please store a pointer to the sas_device in struct scsi_target ->hostdata
in _scsih_target_alloc and avoid the need for this and other runtime
lookups where we have a scsi_device or scsi_target structure available.

> @@ -1324,13 +1377,15 @@ _scsih_target_destroy(struct scsi_target *starget)
>  
>  	spin_lock_irqsave(&ioc->sas_device_lock, flags);
>  	rphy = dev_to_rphy(starget->dev.parent);
> -	sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc,
> +	sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc,
>  	   rphy->identify.sas_address);
>  	if (sas_device && (sas_device->starget == starget) &&
>  	    (sas_device->id == starget->id) &&
>  	    (sas_device->channel == starget->channel))
>  		sas_device->starget = NULL;
>  
> +	if (sas_device)
> +		sas_device_put(sas_device);
>  	spin_unlock_irqrestore(&ioc->sas_device_lock, flags);

.. like this one.

>   out:
> @@ -1386,7 +1441,7 @@ _scsih_slave_alloc(struct scsi_device *sdev)
>  
>  	if (!(sas_target_priv_data->flags & MPT_TARGET_FLAGS_VOLUME)) {
>  		spin_lock_irqsave(&ioc->sas_device_lock, flags);
> -		sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc,
> +		sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc,
>  				sas_target_priv_data->sas_address);
>  		if (sas_device && (sas_device->starget == NULL)) {
>  			sdev_printk(KERN_INFO, sdev,

.. or this one ..

> @@ -1428,10 +1487,13 @@ _scsih_slave_destroy(struct scsi_device *sdev)
>  
>  	if (!(sas_target_priv_data->flags & MPT_TARGET_FLAGS_VOLUME)) {
>  		spin_lock_irqsave(&ioc->sas_device_lock, flags);
> -		sas_device = mpt2sas_scsih_sas_device_find_by_sas_address(ioc,
> +		sas_device = mpt2sas_scsih_sas_device_get_by_sas_address_nolock(ioc,
>  		   sas_target_priv_data->sas_address);
>  		if (sas_device && !sas_target_priv_data->num_luns)
>  			sas_device->starget = NULL;
> +
> +		if (sas_device)
> +			sas_device_put(sas_device);
>  		spin_unlock_irqrestore(&ioc->sas_device_lock, flags);

.. and this, and many more.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/