Re: [PATCH V5 10/17] scsi: ufs: manually add well known logical units
/**
+ * ufshcd_set_queue_depth - set lun queue depth
+ * @sdev: pointer to SCSI device
+ *
+ * Read bLUQueueDepth value and activate scsi tagged command
+ * queueing. For WLUN, queue depth is set to 1. For best-effort
+ * cases (bLUQueueDepth = 0) the queue depth is set to a maximum
+ * value that host can queue.
+ */
+static void ufshcd_set_queue_depth(struct scsi_device *sdev)
I don't think this refactoring belong in here..
@@ -2152,8 +2215,6 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
static int ufshcd_slave_alloc(struct scsi_device *sdev)
{
struct ufs_hba *hba;
- u8 lun_qdepth;
- int ret;
hba = shost_priv(sdev-host);
sdev-tagged_supported = 1;
@@ -2168,20 +2229,8 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
/* REPORT SUPPORTED OPERATION CODES is not supported */
sdev-no_report_opcodes = 1;
- ret = ufshcd_read_unit_desc_param(hba,
- sdev-lun,
- UNIT_DESC_PARAM_LU_Q_DEPTH,
- lun_qdepth,
- sizeof(lun_qdepth));
- if (!ret || !lun_qdepth)
- /* eventually, we can figure out the real queue depth */
- lun_qdepth = hba-nutrs;
- else
- lun_qdepth = min_t(int, lun_qdepth, hba-nutrs);
- dev_dbg(hba-dev, %s: activate tcq with queue depth %d\n,
- __func__, lun_qdepth);
- scsi_activate_tcq(sdev, lun_qdepth);
+ ufshcd_set_queue_depth(sdev);
return 0;
Addinitional all this setup really belongs into -slave_configure.
-slave_alloc is just for allocating any needed per-LUN data structure
before scanning, while -slave_configure is called after a LUN has
finished scanning and is made available. That should be left for a
separate patch, though.
+static int ufshcd_scsi_add_wlus(struct ufs_hba *hba)
+{
+ int ret = 0;
+
+ hba-sdev_ufs_device = __scsi_add_device(hba-host, 0, 0,
+ ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN), NULL);
+ if (IS_ERR(hba-sdev_ufs_device)) {
+ ret = PTR_ERR(hba-sdev_ufs_device);
+ hba-sdev_ufs_device = NULL;
+ goto out;
+ }
Where do you release these references again? It seems like they are
never released on the device removal path.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V5 10/17] scsi: ufs: manually add well known logical units
Comments inline:
-Original Message-
From: Christoph Hellwig [mailto:h...@infradead.org]
Sent: Wednesday, September 24, 2014 9:25 AM
To: Dolev Raviv
Cc: james.bottom...@hansenpartnership.com; h...@infradead.org;
linux-scsi@vger.kernel.org; linux-scsi-ow...@vger.kernel.org;
linux-arm-...@vger.kernel.org; santos...@gmail.com; Subhash Jadavani; Sujit
Reddy Thumma
Subject: Re: [PATCH V5 10/17] scsi: ufs: manually add well known logical
units
/**
+ * ufshcd_set_queue_depth - set lun queue depth
+ * @sdev: pointer to SCSI device
+ *
+ * Read bLUQueueDepth value and activate scsi tagged command
+ * queueing. For WLUN, queue depth is set to 1. For best-effort
+ * cases (bLUQueueDepth = 0) the queue depth is set to a maximum
+ * value that host can queue.
+ */
+static void ufshcd_set_queue_depth(struct scsi_device *sdev)
I don't think this refactoring belong in here..
[Subhash] I agree, This shouldn't belong here. Will fix it in next patch.
@@ -2152,8 +2215,6 @@ static int ufshcd_verify_dev_init(struct ufs_hba
*hba) static int ufshcd_slave_alloc(struct scsi_device *sdev) {
struct ufs_hba *hba;
- u8 lun_qdepth;
- int ret;
hba = shost_priv(sdev-host);
sdev-tagged_supported = 1;
@@ -2168,20 +2229,8 @@ static int ufshcd_slave_alloc(struct scsi_device
*sdev)
/* REPORT SUPPORTED OPERATION CODES is not supported */
sdev-no_report_opcodes = 1;
- ret = ufshcd_read_unit_desc_param(hba,
- sdev-lun,
- UNIT_DESC_PARAM_LU_Q_DEPTH,
- lun_qdepth,
- sizeof(lun_qdepth));
- if (!ret || !lun_qdepth)
- /* eventually, we can figure out the real queue depth */
- lun_qdepth = hba-nutrs;
- else
- lun_qdepth = min_t(int, lun_qdepth, hba-nutrs);
- dev_dbg(hba-dev, %s: activate tcq with queue depth %d\n,
- __func__, lun_qdepth);
- scsi_activate_tcq(sdev, lun_qdepth);
+ ufshcd_set_queue_depth(sdev);
return 0;
Addinitional all this setup really belongs into -slave_configure.
-slave_alloc is just for allocating any needed per-LUN data structure
before scanning, while -slave_configure is called after a LUN has finished
scanning and is made available. That should be left for a separate patch,
though.
[Subhash] Ok, will fix it and I agree it shouldn't be part of this patch.
+static int ufshcd_scsi_add_wlus(struct ufs_hba *hba) {
+ int ret = 0;
+
+ hba-sdev_ufs_device = __scsi_add_device(hba-host, 0, 0,
+ ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN),
NULL);
+ if (IS_ERR(hba-sdev_ufs_device)) {
+ ret = PTR_ERR(hba-sdev_ufs_device);
+ hba-sdev_ufs_device = NULL;
+ goto out;
+ }
Where do you release these references again? It seems like they are never
released on the device removal path.
[Subhash] That's because these are embedded/non-removable UFS devices which
are always present on the board and never get removed. Do you have any
suggestion on how we should handle this?
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 10/17] scsi: ufs: manually add well known logical units
On Wed, Sep 24, 2014 at 09:36:37AM -0700, Subhash Jadavani wrote: Where do you release these references again? It seems like they are never released on the device removal path. [Subhash] That's because these are embedded/non-removable UFS devices which are always present on the board and never get removed. Do you have any suggestion on how we should handle this? Just after the call to scsi_remove_host sounds right to me. scsi_remove_host already removes all regularly scanned devices, but because __scsi_add_device keeps and additional reference it doesn't free those that you added manually. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V5 10/17] scsi: ufs: manually add well known logical units
Just after the call to scsi_remove_host sounds right to me. scsi_remove_host already removes all regularly scanned devices, but because __scsi_add_device keeps and additional reference it doesn't free those that you added manually. Ok, we are calling scsi_remove_host() as part of ufs driver removal so we can remove these manually added devices immediately after it. Next patch revision should fix it. -Original Message- From: 'Christoph Hellwig' [mailto:h...@infradead.org] Sent: Wednesday, September 24, 2014 9:41 AM To: Subhash Jadavani Cc: 'Christoph Hellwig'; 'Dolev Raviv'; james.bottom...@hansenpartnership.com; linux-scsi@vger.kernel.org; linux-scsi-ow...@vger.kernel.org; linux-arm-...@vger.kernel.org; santos...@gmail.com; 'Sujit Reddy Thumma' Subject: Re: [PATCH V5 10/17] scsi: ufs: manually add well known logical units On Wed, Sep 24, 2014 at 09:36:37AM -0700, Subhash Jadavani wrote: Where do you release these references again? It seems like they are never released on the device removal path. [Subhash] That's because these are embedded/non-removable UFS devices which are always present on the board and never get removed. Do you have any suggestion on how we should handle this? Just after the call to scsi_remove_host sounds right to me. scsi_remove_host already removes all regularly scanned devices, but because __scsi_add_device keeps and additional reference it doesn't free those that you added manually. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
