Re: OOPS: unplugging western digital passport drive
Joe or Stanisﾅ^ﾂaw:
I never heard anything back about this. Does the patch fix the crash?
Alan Stern
On Wed, 18 Mar 2015, Alan Stern wrote:
On Wed, 18 Mar 2015, Alan Stern wrote:
On Tue, 17 Mar 2015, Joe Lawrence wrote:
On 03/11/2015 12:25 AM, Stanisław Pitucha wrote:
Hi linux-scsi,
I've got another case of reproducible crash when unplugging western
digital passport drives. This was mentioned before in
http://www.spinics.net/lists/linux-scsi/msg82603.html
Like it says in that thread, the problem is somehow related to the ses
driver. If you remove or blacklist that driver, there won't be any
more crashes.
Looks like I spoke too soon. I think the patch below will fix the
problem. Let me know what happens.
Alan Stern
Index: usb-4.0/drivers/scsi/scsi_pm.c
===
--- usb-4.0.orig/drivers/scsi/scsi_pm.c
+++ usb-4.0/drivers/scsi/scsi_pm.c
@@ -217,14 +217,18 @@ static int sdev_runtime_suspend(struct d
{
const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
struct scsi_device *sdev = to_scsi_device(dev);
- int err;
+ struct device *blk_rpm_dev = sdev-request_queue-dev;
+ int err = 0;
- err = blk_pre_runtime_suspend(sdev-request_queue);
- if (err)
- return err;
+ if (blk_rpm_dev) {
+ err = blk_pre_runtime_suspend(sdev-request_queue);
+ if (err)
+ return err;
+ }
if (pm pm-runtime_suspend)
err = pm-runtime_suspend(dev);
- blk_post_runtime_suspend(sdev-request_queue, err);
+ if (blk_rpm_dev)
+ blk_post_runtime_suspend(sdev-request_queue, err);
return err;
}
@@ -246,12 +250,15 @@ static int sdev_runtime_resume(struct de
{
struct scsi_device *sdev = to_scsi_device(dev);
const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
+ struct device *blk_rpm_dev = sdev-request_queue-dev;
int err = 0;
- blk_pre_runtime_resume(sdev-request_queue);
+ if (blk_rpm_dev)
+ blk_pre_runtime_resume(sdev-request_queue);
if (pm pm-runtime_resume)
err = pm-runtime_resume(dev);
- blk_post_runtime_resume(sdev-request_queue, err);
+ if (blk_rpm_dev)
+ blk_post_runtime_resume(sdev-request_queue, err);
return err;
}
--
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: [RFC PATCH 1/2] lpfc: add target infrastructure
Hannes Reinecke wrote: On 06/14/2015 05:20 PM, Sebastian Herbszt wrote: Add target infrastructure. Signed-off-by: Sebastian Herbszt herb...@gmx.de --- Hmm. And that hooks into _which_ target mode infrastructure? Currently it is SCST with the lpfc_scst module. I still need to figure out how to plug it into LIO/TCM. Cheers, Hannes Sebastian -- 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 1/1] scsi: use kzalloc for allocating one thing
On 06/18/2015 06:36 AM, Maninder Singh wrote:
Use kzalloc rather than kcalloc(1,...) for allocating one thing
Signed-off-by: Maninder Singh maninder...@samsung.com
Reviewed-by: Vaneet Narang v.nar...@samsung.com
---
drivers/scsi/mvsas/mv_init.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index d40d734..65e47eb 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -558,7 +558,7 @@ static int mvs_pci_init(struct pci_dev *pdev, const
struct pci_device_id *ent)
chip = mvs_chips[ent-driver_data];
SHOST_TO_SAS_HA(shost) =
- kcalloc(1, sizeof(struct sas_ha_struct), GFP_KERNEL);
+ kzalloc(sizeof(struct sas_ha_struct), GFP_KERNEL);
if (!SHOST_TO_SAS_HA(shost)) {
kfree(shost);
rc = -ENOMEM;
Reviewed-by: Hannes Reinecke h...@suse.de
Cheers,
Hannes
--
Dr. Hannes ReineckezSeries Storage
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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
[for 4.1 PATCH resend] libsas: fix sysfs group not found warnings at port teardown time
Praveen reports:
After some debugging this is what I have found
sas_phye_loss_of_signal gets triggered on phy_event from mvsas
sas_phye_loss_of_signal calls sas_deform_port
sas_deform_port posts a DISCE_DESTRUCT event
(sas_unregister_domain_devices- sas_unregister_dev)
sas_deform_port calls sas_port_delete
sas_port_delete calls sas_port_delete_link
sysfs_remove_group: kobject 'port-X:Y'
sas_port_delete calls device_del
sysfs_remove_group: kobject 'port-X:Y'
sas_destruct_devices gets triggered for the destruct event (DISCE_DESTRUCT)
sas_destruct_devices calls sas_rphy_delete
sas_rphy_delete calls scsi_remove_device
scsi_remove_device calls __scsi_remove_device
__scsi_remove_device calls bsg_unregister_queue
bsg_unregister_queue - device_unregister -
device_del - sysfs_remove_group: kobject 'X:0:0:0'
Since X:0:0:0 falls under port-X:Y (which got deleted during
sas_port_delete), this call results in the warning. All the later
warnings in the dmesg output I sent earlier are trying to delete objects
under port-X:Y. Since port-X:Y got recursively deleted, all these calls
result in warnings. Since, the PHY and DISC events are processed in two
different work queues (and one triggers the other), is there any way
other than checking if the object exists in sysfs (in device_del) before
deleting?
WARNING: CPU: 2 PID: 6 at fs/sysfs/group.c:219 device_del+0x40/0x1c0()
sysfs group 818b97e0 not found for kobject '2:0:4:0'
[..]
CPU: 2 PID: 6 Comm: kworker/u8:0 Tainted: PW O
3.16.7-ckt9-logicube-ng.3 #1
Hardware name: To be filled by O.E.M. To be filled by O.E.M./VT6085, BIOS
4.6.5 01/23/2015
Workqueue: scsi_wq_2 sas_destruct_devices [libsas]
0009 8151cd18 88011b35bcd8 810687b7
88011a661400 88011b35bd28 8800c6e5e968 88028810
8800c89f2c00 8106881c 81733b68 0028
Call Trace:
[8151cd18] ? dump_stack+0x41/0x51
[810687b7] ? warn_slowpath_common+0x77/0x90
[8106881c] ? warn_slowpath_fmt+0x4c/0x50
[813ad2d0] ? device_del+0x40/0x1c0
[813ad46a] ? device_unregister+0x1a/0x70
[812a535e] ? bsg_unregister_queue+0x5e/0xb0
[a00781a9] ? __scsi_remove_device+0xa9/0xd0 [scsi_mod]
It appears we've always been double deleting the devices below sas_port,
but recent sysfs changes now exposes this problem. Libsas should delete
all the devices from rphy down before deleting the parent port.
Cc: sta...@vger.kernel.org
Reported-by: Praveen Murali pmur...@logicube.com
Tested-by: Praveen Murali pmur...@logicube.com
Signed-off-by: Dan Williams dan.j.willi...@intel.com
---
drivers/scsi/libsas/sas_discover.c |6 +++---
drivers/scsi/libsas/sas_port.c |1 -
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/libsas/sas_discover.c
b/drivers/scsi/libsas/sas_discover.c
index 60de66252fa2..a4db770fe8b0 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -362,11 +362,14 @@ static void sas_destruct_devices(struct work_struct *work)
clear_bit(DISCE_DESTRUCT, port-disc.pending);
list_for_each_entry_safe(dev, n, port-destroy_list, disco_list_node) {
+ struct sas_port *sas_port =
dev_to_sas_port(dev-rphy-dev.parent);
+
list_del_init(dev-disco_list_node);
sas_remove_children(dev-rphy-dev);
sas_rphy_delete(dev-rphy);
sas_unregister_common_dev(port, dev);
+ sas_port_delete(sas_port);
}
}
@@ -400,9 +403,6 @@ void sas_unregister_domain_devices(struct asd_sas_port
*port, int gone)
list_for_each_entry_safe(dev, n, port-disco_list, disco_list_node)
sas_unregister_dev(port, dev);
-
- port-port-rphy = NULL;
-
}
void sas_device_set_phy(struct domain_device *dev, struct sas_port *port)
diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
index d3c5297c6c89..9a25ae3a52a4 100644
--- a/drivers/scsi/libsas/sas_port.c
+++ b/drivers/scsi/libsas/sas_port.c
@@ -219,7 +219,6 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone)
if (port-num_phys == 1) {
sas_unregister_domain_devices(port, gone);
- sas_port_delete(port-port);
port-port = NULL;
} else {
sas_port_delete_phy(port-port, phy-phy);
--
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
[PATCH 1/1] scsi: use kzalloc for allocating one thing
Use kzalloc rather than kcalloc(1,...) for allocating one thing
Signed-off-by: Maninder Singh maninder...@samsung.com
Reviewed-by: Vaneet Narang v.nar...@samsung.com
---
drivers/scsi/mvsas/mv_init.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index d40d734..65e47eb 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -558,7 +558,7 @@ static int mvs_pci_init(struct pci_dev *pdev, const struct
pci_device_id *ent)
chip = mvs_chips[ent-driver_data];
SHOST_TO_SAS_HA(shost) =
- kcalloc(1, sizeof(struct sas_ha_struct), GFP_KERNEL);
+ kzalloc(sizeof(struct sas_ha_struct), GFP_KERNEL);
if (!SHOST_TO_SAS_HA(shost)) {
kfree(shost);
rc = -ENOMEM;
--
1.7.1
--
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
[PATCH 1/1] scsi: Initialize sdp after NULL check of cmnd
Currently cmnd pointer is already dereferenced before NULL check
and thus getting below warning in static analysis:
warn: variable dereferenced before check 'cmnd'
So initialize struct scsi_device *sdp after NULL check
of cmnd
Signed-off-by: Maninder Singh maninder...@samsung.com
Reviewed-by: Akhilesh Kumar akhiles...@samsung.com
---
drivers/scsi/scsi_debug.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1f8e2dc..bb97a5a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3942,7 +3942,7 @@ schedule_resp(struct scsi_cmnd *cmnd, struct
sdebug_dev_info *devip,
unsigned long iflags;
int k, num_in_q, qdepth, inject;
struct sdebug_queued_cmd *sqcp = NULL;
- struct scsi_device *sdp = cmnd-device;
+ struct scsi_device *sdp;
if (NULL == cmnd || NULL == devip) {
pr_warn(%s: called with NULL cmnd or devip pointer\n,
@@ -3950,6 +3950,8 @@ schedule_resp(struct scsi_cmnd *cmnd, struct
sdebug_dev_info *devip,
/* no particularly good error to report back */
return SCSI_MLQUEUE_HOST_BUSY;
}
+
+ sdp = cmnd-device;
if ((scsi_result) (SCSI_DEBUG_OPT_NOISE scsi_debug_opts))
sdev_printk(KERN_INFO, sdp, %s: non-zero result=0x%x\n,
__func__, scsi_result);
--
1.7.1
--
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 v1 02/20] [SCSI] mpt3sas: Get IOC_FACTS information using handshake protocol only after HBA card gets into READY or Operational state.
On 06/17/2015 11:10 AM, Sreekanth Reddy wrote: Driver initialization fails if driver tries to send IOC facts request message when the IOC is in reset or in a fault state. This patch will make sure that 1.Driver to send IOC facts request message only if HBA is in operational or ready state. 2.If IOC is in fault state, a diagnostic reset would be issued. 3.If IOC is in reset state then driver will wait for 10 seconds to exit out of reset state. If the HBA continues to be in reset state, then the HBA wouldn't be claimed by the driver. Changes in v1: If PCI Recovery is on then return with -EFAULT in the function _base_wait_for_iocstate(). Signed-off-by: Sreekanth Reddy sreekanth.re...@avagotech.com Reviewed-by: Tomas Henzl the...@redhat.com Tomas -- 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 1/1] scsi: Initialize sdp after NULL check of cmnd
On Wed, Jun 17, 2015 at 04:51:07PM +0530, Maninder Singh wrote:
Currently cmnd pointer is already dereferenced before NULL check
and thus getting below warning in static analysis:
warn: variable dereferenced before check 'cmnd'
So initialize struct scsi_device *sdp after NULL check
of cmnd
Signed-off-by: Maninder Singh maninder...@samsung.com
Reviewed-by: Akhilesh Kumar akhiles...@samsung.com
---
drivers/scsi/scsi_debug.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 1f8e2dc..bb97a5a 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3942,7 +3942,7 @@ schedule_resp(struct scsi_cmnd *cmnd, struct
sdebug_dev_info *devip,
unsigned long iflags;
int k, num_in_q, qdepth, inject;
struct sdebug_queued_cmd *sqcp = NULL;
- struct scsi_device *sdp = cmnd-device;
+ struct scsi_device *sdp;
if (NULL == cmnd || NULL == devip) {
pr_warn(%s: called with NULL cmnd or devip pointer\n,
@@ -3950,6 +3950,8 @@ schedule_resp(struct scsi_cmnd *cmnd, struct
sdebug_dev_info *devip,
/* no particularly good error to report back */
return SCSI_MLQUEUE_HOST_BUSY;
}
+
+ sdp = cmnd-device;
if ((scsi_result) (SCSI_DEBUG_OPT_NOISE scsi_debug_opts))
sdev_printk(KERN_INFO, sdp, %s: non-zero result=0x%x\n,
__func__, scsi_result);
--
1.7.1
--
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
Reviewed-by: Johannes Thumshirn jthumsh...@suse.de
--
Johannes Thumshirn Storage
jthumsh...@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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: [RFC PATCH 1/2] lpfc: add target infrastructure
On 06/14/2015 05:20 PM, Sebastian Herbszt wrote: Add target infrastructure. Signed-off-by: Sebastian Herbszt herb...@gmx.de --- Hmm. And that hooks into _which_ target mode infrastructure? Cheers, Hannes -- Dr. Hannes ReineckezSeries Storage h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- 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 3/6] target: use 'se_dev_entry' when allocating UAs
On 06/17/2015 08:06 AM, Nicholas A. Bellinger wrote:
Hey Hannes,
Apologies for the delayed follow-up on these, one comment below.
On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote:
We need to use 'se_dev_entry' as argument when allocating
UAs, otherwise we'll never see any UAs for an implicit
ALUA state transition triggered from userspace.
Signed-off-by: Hannes Reinecke h...@suse.de
---
drivers/target/target_core_alua.c | 27 ++-
drivers/target/target_core_pr.c| 31 +--
drivers/target/target_core_transport.c | 18 --
drivers/target/target_core_ua.c| 23 +++
drivers/target/target_core_ua.h| 2 +-
5 files changed, 59 insertions(+), 42 deletions(-)
SNIP
diff --git a/drivers/target/target_core_pr.c
b/drivers/target/target_core_pr.c
index 436e30b..bb28a97 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -125,6 +125,25 @@ static struct t10_pr_registration
*core_scsi3_locate_pr_reg(struct se_device *,
struct se_node_acl *, struct se_session
*);
static void core_scsi3_put_pr_reg(struct t10_pr_registration *);
+static void core_scsi3_pr_ua_allocate(struct se_node_acl *nacl,
+ u32 unpacked_lun, u8 asc, u8 ascq)
+{
+struct se_dev_entry *deve;
+
+if (!nacl)
+return;
+
+rcu_read_lock();
+deve = target_nacl_find_deve(nacl, unpacked_lun);
+if (!deve) {
+rcu_read_unlock();
+return;
+}
+
+core_scsi3_ua_allocate(deve, asc, ascq);
+rcu_read_unlock();
+}
+
This should be common for TCM_RESERVATION_CONFLICT case outside of PR
code too.
Any objections for squashing the following into your original patch..?
Thank you,
--nab
[ .. ]
None at all.
Do go ahead.
Cheers,
Hannes
--
Dr. Hannes ReineckezSeries Storage
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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] mpt2sas: setpci reset kernel panic fix
On Wed, Jun 17, 2015 at 11:37:53AM +0530, Nagarajkumar Narayanan wrote:
Problem Description:
https://bugzilla.kernel.org/show_bug.cgi?id=95101
Due to lack of synchronization between ioctl, BRM status access, pci
resource removal kernel oops happen as ioctl path and BRM status
access path still tries to access the removed resources
kernel: BUG: unable to handle kernel paging request at c900171e
Oops: [#1] SMP
Patch Description:
Two locks added to provide syncrhonization
1. pci_access_mutex: Mutex to synchronize ioctl,sysfs show path and
pci resource handling. PCI resource freeing will lead to free
vital hardware/memory resource, which might be in use by cli/sysfs
path functions resulting in Null pointer reference followed by kernel
crash. To avoid the above race condition we use mutex syncrhonization
which ensures the syncrhonization between cli/sysfs_show path
2. spinlock on list operations over IOCs
Case: when multiple warpdrive cards(IOCs) are in use
Each IOC will added to the ioc list stucture on initialization.
Watchdog threads run at regular intervals to check IOC for any
fault conditions which will trigger the dead_ioc thread to
deallocate pci resource, resulting deleting the IOC netry from list,
this deletion need to protected by spinlock to enusre that
ioc removal is syncrhonized, if not synchronized it might lead to
list_del corruption as the ioc list is traversed in cli path
please find the patch to apply on scsi/mpt2sas driver
http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
From ba692140278e6e2b660896c32206b26dac98d215 Mon Sep 17 00:00:00 2001
From: Nagarajkumar Narayanan nagarajkumar.naraya...@seagate.com
Date: Thu, 19 Mar 2015 12:02:07 +0530
Subject: [PATCH] mpt2sas setpci kernel oops fix
Signed-off-by: Nagarajkumar Narayanan nagarajkumar.naraya...@seagate.com
---
drivers/scsi/mpt2sas/mpt2sas_base.c | 10 +++
drivers/scsi/mpt2sas/mpt2sas_base.h | 20 +-
drivers/scsi/mpt2sas/mpt2sas_ctl.c | 48 +
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 32 ++-
4 files changed, 102 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
b/drivers/scsi/mpt2sas/mpt2sas_base.c
index 11248de..d2a498c 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -108,13 +108,18 @@ _scsih_set_fwfault_debug(const char *val, struct
kernel_param *kp)
{
int ret = param_set_int(val, kp);
struct MPT2SAS_ADAPTER *ioc;
+ unsigned long flags;
if (ret)
return ret;
+ /* global ioc spinlock to protect controller list on list operations */
+ mpt2sas_initialize_gioc_lock();
printk(KERN_INFO setting fwfault_debug(%d)\n, mpt2sas_fwfault_debug);
+ spin_lock_irqsave(gioc_lock, flags);
list_for_each_entry(ioc, mpt2sas_ioc_list, list)
ioc-fwfault_debug = mpt2sas_fwfault_debug;
+ spin_unlock_irqrestore(gioc_lock, flags);
return 0;
}
@@ -4436,6 +4441,9 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
__func__));
if (ioc-chip_phys ioc-chip) {
+ /* synchronizing freeing resource with pci_access_mutex lock */
+ if (ioc-is_warpdrive)
+ mutex_lock(ioc-pci_access_mutex);
_base_mask_interrupts(ioc);
ioc-shost_recovery = 1;
_base_make_ioc_ready(ioc, CAN_SLEEP, SOFT_RESET);
@@ -4454,6 +4462,8 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
pci_disable_pcie_error_reporting(pdev);
pci_disable_device(pdev);
}
+ if (ioc-is_warpdrive)
+ mutex_unlock(ioc-pci_access_mutex);
return;
}
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
b/drivers/scsi/mpt2sas/mpt2sas_base.h
index caff8d1..a0d26f0 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -799,6 +799,12 @@ typedef void (*MPT2SAS_FLUSH_RUNNING_CMDS)(struct
MPT2SAS_ADAPTER *ioc);
* @delayed_tr_list: target reset link list
* @delayed_tr_volume_list: volume target reset link list
* @@temp_sensors_count: flag to carry the number of temperature sensors
+ * @pci_access_mutex: Mutex to synchronize ioctl,sysfs show path and
+ * pci resource handling. PCI resource freeing will lead to free
+ * vital hardware/memory resource, which might be in use by cli/sysfs
+ * path functions resulting in Null pointer reference followed by kernel
+ * crash. To avoid the above race condition we use mutex syncrhonization
+ * which ensures the syncrhonization between cli/sysfs_show path
*/
struct MPT2SAS_ADAPTER {
struct list_head list;
@@ -1015,6 +1021,7 @@ struct MPT2SAS_ADAPTER {
u8 mfg_pg10_hide_flag;
u8 hide_drives;
+ struct mutex pci_access_mutex;
};
typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8
msix_index,
@@ -1023,6 +1030,17 @@ typedef u8 (*MPT_CALLBACK)(struct
MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
/* base shared API */
extern struct list_head
[PATCH] mpt2sas: setpci reset kernel panic fix
Problem Description:
https://bugzilla.kernel.org/show_bug.cgi?id=95101
Due to lack of synchronization between ioctl, BRM status access, pci
resource removal kernel oops happen as ioctl path and BRM status
access path still tries to access the removed resources
kernel: BUG: unable to handle kernel paging request at c900171e
Oops: [#1] SMP
Patch Description:
Two locks added to provide syncrhonization
1. pci_access_mutex: Mutex to synchronize ioctl,sysfs show path and
pci resource handling. PCI resource freeing will lead to free
vital hardware/memory resource, which might be in use by cli/sysfs
path functions resulting in Null pointer reference followed by kernel
crash. To avoid the above race condition we use mutex syncrhonization
which ensures the syncrhonization between cli/sysfs_show path
2. spinlock on list operations over IOCs
Case: when multiple warpdrive cards(IOCs) are in use
Each IOC will added to the ioc list stucture on initialization.
Watchdog threads run at regular intervals to check IOC for any
fault conditions which will trigger the dead_ioc thread to
deallocate pci resource, resulting deleting the IOC netry from list,
this deletion need to protected by spinlock to enusre that
ioc removal is syncrhonized, if not synchronized it might lead to
list_del corruption as the ioc list is traversed in cli path
please find the patch to apply on scsi/mpt2sas driver
http://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
From ba692140278e6e2b660896c32206b26dac98d215 Mon Sep 17 00:00:00 2001
From: Nagarajkumar Narayanan nagarajkumar.naraya...@seagate.com
Date: Thu, 19 Mar 2015 12:02:07 +0530
Subject: [PATCH] mpt2sas setpci kernel oops fix
Signed-off-by: Nagarajkumar Narayanan nagarajkumar.naraya...@seagate.com
---
drivers/scsi/mpt2sas/mpt2sas_base.c | 10 +++
drivers/scsi/mpt2sas/mpt2sas_base.h | 20 +-
drivers/scsi/mpt2sas/mpt2sas_ctl.c | 48 +
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 32 ++-
4 files changed, 102 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
b/drivers/scsi/mpt2sas/mpt2sas_base.c
index 11248de..d2a498c 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -108,13 +108,18 @@ _scsih_set_fwfault_debug(const char *val, struct
kernel_param *kp)
{
int ret = param_set_int(val, kp);
struct MPT2SAS_ADAPTER *ioc;
+ unsigned long flags;
if (ret)
return ret;
+ /* global ioc spinlock to protect controller list on list operations */
+ mpt2sas_initialize_gioc_lock();
printk(KERN_INFO setting fwfault_debug(%d)\n, mpt2sas_fwfault_debug);
+ spin_lock_irqsave(gioc_lock, flags);
list_for_each_entry(ioc, mpt2sas_ioc_list, list)
ioc-fwfault_debug = mpt2sas_fwfault_debug;
+ spin_unlock_irqrestore(gioc_lock, flags);
return 0;
}
@@ -4436,6 +4441,9 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
__func__));
if (ioc-chip_phys ioc-chip) {
+ /* synchronizing freeing resource with pci_access_mutex lock */
+ if (ioc-is_warpdrive)
+ mutex_lock(ioc-pci_access_mutex);
_base_mask_interrupts(ioc);
ioc-shost_recovery = 1;
_base_make_ioc_ready(ioc, CAN_SLEEP, SOFT_RESET);
@@ -4454,6 +4462,8 @@ mpt2sas_base_free_resources(struct MPT2SAS_ADAPTER *ioc)
pci_disable_pcie_error_reporting(pdev);
pci_disable_device(pdev);
}
+ if (ioc-is_warpdrive)
+ mutex_unlock(ioc-pci_access_mutex);
return;
}
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
b/drivers/scsi/mpt2sas/mpt2sas_base.h
index caff8d1..a0d26f0 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -799,6 +799,12 @@ typedef void (*MPT2SAS_FLUSH_RUNNING_CMDS)(struct
MPT2SAS_ADAPTER *ioc);
* @delayed_tr_list: target reset link list
* @delayed_tr_volume_list: volume target reset link list
* @@temp_sensors_count: flag to carry the number of temperature sensors
+ * @pci_access_mutex: Mutex to synchronize ioctl,sysfs show path and
+ * pci resource handling. PCI resource freeing will lead to free
+ * vital hardware/memory resource, which might be in use by cli/sysfs
+ * path functions resulting in Null pointer reference followed by kernel
+ * crash. To avoid the above race condition we use mutex syncrhonization
+ * which ensures the syncrhonization between cli/sysfs_show path
*/
struct MPT2SAS_ADAPTER {
struct list_head list;
@@ -1015,6 +1021,7 @@ struct MPT2SAS_ADAPTER {
u8 mfg_pg10_hide_flag;
u8 hide_drives;
+ struct mutex pci_access_mutex;
};
typedef u8 (*MPT_CALLBACK)(struct MPT2SAS_ADAPTER *ioc, u16 smid, u8
msix_index,
@@ -1023,6 +1030,17 @@ typedef u8 (*MPT_CALLBACK)(struct
MPT2SAS_ADAPTER *ioc, u16 smid, u8 msix_index,
/* base shared API */
extern struct list_head mpt2sas_ioc_list;
+/* spinlock on list operations over IOCs
++ * Case: when multiple warpdrive cards(IOCs) are in use
++ * Each IOC will added to the ioc list stucture on initialization.
++ * Watchdog
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote: On Sun, Jun 7, 2015 at 10:32 AM, yga...@codeaurora.org wrote: 2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: [...] If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens always), then the calling to of_platform_populate() which is added, guarantees that ufs-qcom probe will be called and finish, before ufshcd_pltfrm probe continues. so ufs_variant device is always there, and ready. I think it means we are safe - since either way, we make sure ufs-qcom probe will be called and finish before dealing with ufs_variant device in ufshcd_pltfrm probe. This is due to the fact that you have 2 platform drivers. You should only have 1 (and 1 node). If you really think you need 2, then you should do like many other common *HCIs do and make the base UFS driver a set of library functions that drivers can use or call. Look at EHCI, AHCI, SDHCI, etc. for inspiration. Hi Rob, We did look at SDHCI and decided to go with this design due to its simplicity and lack of library functions. Yaniv described the proper flow of probing and, as we understand things, it is guaranteed to work as designed. Furthermore, the design of having a subcore in the dts is used in the Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example - both dwc3-msm and dwc3-exynox invoke the probing function in core.c (i.e. the shared underlying Synopsys USB dwc3 core) by calling of_platform_populate(). That binding has the same problem. Please don't propagate that. There is no point in a sub-node in this case. Do you see a benefit in the SDHCi implementation? Yes, it does not let the kernel driver design dictate the hardware description. Rob Hi Rob, We appear to be having a philosophical disagreement on the practicality of designing the ufshcd variant's implementation - in other words, we disagree on the proper design pattern to follow here. If I understand correctly, you are concerned with a design pattern wherein a generic implementation is wrapped - at the device-tree level - in a variant implementation. The main reason for your concern is that you don't want the kernel driver design dictate the hardware description. We considered this point when we suggested our implementation (both before and after you raised it) and reached the conclusion that - while an important consideration - it should not be the prevailing one. I believe that you will agree once you read the reasoning. What guided us was the following: 1. Keep our change minimal. 2. Keep our patch in line with known design patterns in the kernel. 3. Have our patch extend the existing solution rather than reinvent it. It is the 3rd point that is most important to this discussion, since UFS has already been deployed by various vendors and is used by OEM. Changing ufshcd to a set of library functions that would be called by variants would necessarily introduce a significant change to the code flow in many places and would pose a backward compatibility issue. By using the tried and tested pattern of subnodes in the dts we were able to keep the change simple, succinct, understandable, maintainable and backward compatible. In fact, the entire logic tying of the generic implementation to the variant takes ~20 lines of code - both short and elegant. As to your concern, while I understand it and understand the underlying logic, I don't think that it should outweigh the other considerations that I outlined here. Dov QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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 0/6] target: Update UA handling
On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote: Hi Nic, lio-target is very minimalistic when it comes to generate UAs; primarily they are generated for persistent reservations, but generic changes tend to be ignored. This patchset updates the UA handling and generates UA for internal state changes (REPORTED LUNS DATA CHANGED, INQUIRY DATA CHANGED, and LUN RESET OCCURRED). Funnily enough this triggers some issues with the SCSI stack; I'll be sending out patches for that, too. Hannes Reinecke (6): target_core_alua: Correct UA handling when switching states target: Remove 'ua_nacl' pointer from se_ua structure target: use 'se_dev_entry' when allocating UAs target: Send UA on ALUA target port group change target: Send UA upon LUN RESET tmr completion target: Send UA when changing LUN inventory drivers/target/target_core_alua.c | 56 +- drivers/target/target_core_device.c| 26 +++- drivers/target/target_core_pr.c| 31 +++ drivers/target/target_core_transport.c | 29 ++ drivers/target/target_core_ua.c| 24 ++- drivers/target/target_core_ua.h| 5 ++- include/target/target_core_base.h | 1 - 7 files changed, 121 insertions(+), 51 deletions(-) Applied to target-pending/for-next, with the extra incremental patch for a common target_ua_alloc_lun() caller. Btw, very happy to see REPORTED_LUNS_DATA_HAS_CHANGED support include for v4.2-rc1 code. 8-) Thanks Hannes! --nab -- 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 0/6] target: Update UA handling
On Wed, 2015-06-17 at 08:25 +0200, Hannes Reinecke wrote: On 06/17/2015 08:10 AM, Nicholas A. Bellinger wrote: On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote: Hi Nic, lio-target is very minimalistic when it comes to generate UAs; primarily they are generated for persistent reservations, but generic changes tend to be ignored. This patchset updates the UA handling and generates UA for internal state changes (REPORTED LUNS DATA CHANGED, INQUIRY DATA CHANGED, and LUN RESET OCCURRED). Funnily enough this triggers some issues with the SCSI stack; I'll be sending out patches for that, too. Hannes Reinecke (6): target_core_alua: Correct UA handling when switching states target: Remove 'ua_nacl' pointer from se_ua structure target: use 'se_dev_entry' when allocating UAs target: Send UA on ALUA target port group change target: Send UA upon LUN RESET tmr completion target: Send UA when changing LUN inventory drivers/target/target_core_alua.c | 56 +- drivers/target/target_core_device.c| 26 +++- drivers/target/target_core_pr.c| 31 +++ drivers/target/target_core_transport.c | 29 ++ drivers/target/target_core_ua.c| 24 ++- drivers/target/target_core_ua.h| 5 ++- include/target/target_core_base.h | 1 - 7 files changed, 121 insertions(+), 51 deletions(-) Applied to target-pending/for-next, with the extra incremental patch for a common target_ua_alloc_lun() caller. Btw, very happy to see REPORTED_LUNS_DATA_HAS_CHANGED support include for v4.2-rc1 code. 8-) Yeah; I needed a quick testbed for my ALUA update, and thought that tcm_loop would fit the bill. As it turned out, not quite. Hence the patches. BTW: the main issue I have with current lio-target is that you can only configure it _after_ the target has been enabled. IE if you want to add another ALUA state you have to create another TPG, and set this to the required ALUA state. But you can modify the TPG allegiance only _after_ the LUN has been created and is visible to the host. Which means that the initiator inevitably sees both states, and it's impossible to have the LUN start off with a different than the default ALUA state. (This is especially important if one would want to test the READ CAPACITY support in ALUA standby state). Would you be okay with changing that? Sounds like a reasonable case to support. No objections to allowing default ALUA access state to be changed, ahead of actual se_device configfs symlink to fabric se_lun export. -- 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
[PATCH v1 02/20] [SCSI] mpt3sas: Get IOC_FACTS information using handshake protocol only after HBA card gets into READY or Operational state.
Driver initialization fails if driver tries to send IOC facts request message
when the IOC is in reset or in a fault state.
This patch will make sure that
1.Driver to send IOC facts request message only if HBA is in operational or
ready state.
2.If IOC is in fault state, a diagnostic reset would be issued.
3.If IOC is in reset state then driver will wait for 10 seconds to exit out of
reset state.
If the HBA continues to be in reset state, then the HBA wouldn't be claimed
by the driver.
Changes in v1:
If PCI Recovery is on then return with -EFAULT in the function
_base_wait_for_iocstate().
Signed-off-by: Sreekanth Reddy sreekanth.re...@avagotech.com
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 68 +
1 file changed, 68 insertions(+)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index fe82092..2386e4b 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3287,6 +3287,9 @@ _base_wait_on_iocstate(struct MPT3SAS_ADAPTER *ioc, u32
ioc_state, int timeout,
* Notes: MPI2_HIS_IOC2SYS_DB_STATUS - set to one when IOC writes to doorbell.
*/
static int
+_base_diag_reset(struct MPT3SAS_ADAPTER *ioc, int sleep_flag);
+
+static int
_base_wait_for_doorbell_int(struct MPT3SAS_ADAPTER *ioc, int timeout,
int sleep_flag)
{
@@ -3829,6 +3832,64 @@ _base_get_port_facts(struct MPT3SAS_ADAPTER *ioc, int
port, int sleep_flag)
}
/**
+ * _base_wait_for_iocstate - Wait until the card is in READY or OPERATIONAL
+ * @ioc: per adapter object
+ * @timeout:
+ * @sleep_flag: CAN_SLEEP or NO_SLEEP
+ *
+ * Returns 0 for success, non-zero for failure.
+ */
+static int
+_base_wait_for_iocstate(struct MPT3SAS_ADAPTER *ioc, int timeout,
+ int sleep_flag)
+{
+ u32 ioc_state;
+ int rc;
+
+ dinitprintk(ioc, printk(MPT3SAS_FMT %s\n, ioc-name,
+ __func__));
+
+ if (ioc-pci_error_recovery) {
+ dfailprintk(ioc, printk(MPT3SAS_FMT
+ %s: host in pci error recovery\n, ioc-name, __func__));
+ return -EFAULT;
+ }
+
+ ioc_state = mpt3sas_base_get_iocstate(ioc, 0);
+ dhsprintk(ioc, printk(MPT3SAS_FMT %s: ioc_state(0x%08x)\n,
+ ioc-name, __func__, ioc_state));
+
+ if (((ioc_state MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_READY) ||
+ (ioc_state MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_OPERATIONAL)
+ return 0;
+
+ if (ioc_state MPI2_DOORBELL_USED) {
+ dhsprintk(ioc, printk(MPT3SAS_FMT
+ unexpected doorbell active!\n, ioc-name));
+ goto issue_diag_reset;
+ }
+
+ if ((ioc_state MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_FAULT) {
+ mpt3sas_base_fault_info(ioc, ioc_state
+ MPI2_DOORBELL_DATA_MASK);
+ goto issue_diag_reset;
+ }
+
+ ioc_state = _base_wait_on_iocstate(ioc, MPI2_IOC_STATE_READY,
+ timeout, sleep_flag);
+ if (ioc_state) {
+ dfailprintk(ioc, printk(MPT3SAS_FMT
+ %s: failed going to ready state (ioc_state=0x%x)\n,
+ ioc-name, __func__, ioc_state));
+ return -EFAULT;
+ }
+
+ issue_diag_reset:
+ rc = _base_diag_reset(ioc, sleep_flag);
+ return rc;
+}
+
+/**
* _base_get_ioc_facts - obtain ioc facts reply and save in ioc
* @ioc: per adapter object
* @sleep_flag: CAN_SLEEP or NO_SLEEP
@@ -3846,6 +3907,13 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc, int
sleep_flag)
dinitprintk(ioc, pr_info(MPT3SAS_FMT %s\n, ioc-name,
__func__));
+ r = _base_wait_for_iocstate(ioc, 10, sleep_flag);
+ if (r) {
+ dfailprintk(ioc, printk(MPT3SAS_FMT
+ %s: failed getting to correct state\n,
+ ioc-name, __func__));
+ return r;
+ }
mpi_reply_sz = sizeof(Mpi2IOCFactsReply_t);
mpi_request_sz = sizeof(Mpi2IOCFactsRequest_t);
memset(mpi_request, 0, mpi_request_sz);
--
2.0.2
--
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
[PATCH] cxgb4i: Fix neigh entry leak
When csk-atid returned by cxgb4_alloc_atid() is less than zero,
init_act_open() directly returns with -EINVAL. But as init_act_open()
ever invokes dst_neigh_lookup() before it calls cxgb4_alloc_atid(),
this leads to the leak of neigh entry searched by dst_neigh_lookup().
Signed-off-by: Ying Xue ying@windriver.com
---
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index dd00e5f..c449d2a 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -1393,7 +1393,7 @@ static int init_act_open(struct cxgbi_sock *csk)
csk-atid = cxgb4_alloc_atid(lldi-tids, csk);
if (csk-atid 0) {
pr_err(%s, NO atid available.\n, ndev-name);
- return -EINVAL;
+ goto rel_resource;
}
cxgbi_sock_set_flag(csk, CTPF_HAS_ATID);
cxgbi_sock_get(csk);
--
1.7.9.5
--
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 02/20] [SCSI] mpt3sas: Get IOC_FACTS information using handshake protocol only after HBA card gets into READY or Operational state.
On Tue, Jun 16, 2015 at 9:29 PM, Tomas Henzl the...@redhat.com wrote:
On 06/12/2015 11:42 AM, Sreekanth Reddy wrote:
Driver initialization fails if driver tries to send IOC facts request
message when the IOC is in reset or in a fault state.
This patch will make sure that
1.Driver to send IOC facts request message only if HBA is in operational or
ready state.
2.If IOC is in fault state, a diagnostic reset would be issued.
3.If IOC is in reset state then driver will wait for 10 seconds to exit out
of reset state.
If the HBA continues to be in reset state, then the HBA wouldn't be
claimed by the driver.
Signed-off-by: Sreekanth Reddy sreekanth.re...@avagotech.com
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 65
+
1 file changed, 65 insertions(+)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index c13a365..ce57320 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3169,6 +3169,9 @@ _base_wait_on_iocstate(struct MPT3SAS_ADAPTER *ioc,
u32 ioc_state, int timeout,
* Notes: MPI2_HIS_IOC2SYS_DB_STATUS - set to one when IOC writes to
doorbell.
*/
static int
+_base_diag_reset(struct MPT3SAS_ADAPTER *ioc, int sleep_flag);
+
+static int
_base_wait_for_doorbell_int(struct MPT3SAS_ADAPTER *ioc, int timeout,
int sleep_flag)
{
@@ -3711,6 +3714,61 @@ _base_get_port_facts(struct MPT3SAS_ADAPTER *ioc, int
port, int sleep_flag)
}
/**
+ * _base_wait_for_iocstate - Wait until the card is in READY or OPERATIONAL
+ * @ioc: per adapter object
+ * @timeout:
+ * @sleep_flag: CAN_SLEEP or NO_SLEEP
+ *
+ * Returns 0 for success, non-zero for failure.
+ */
+static int
+_base_wait_for_iocstate(struct MPT3SAS_ADAPTER *ioc, int timeout,
+ int sleep_flag)
+{
+ u32 ioc_state;
+ int rc;
+
+ dinitprintk(ioc, printk(MPT3SAS_FMT %s\n, ioc-name,
+ __func__));
+
+ if (ioc-pci_error_recovery)
+ return 0;
Hi Sreekanth, isn't that^ an error condition - 'return -EFAULT;'
would be better?
Tomas
Accepted. I will post the next version of this patch with this change.
Thanks,
Sreekanth
+
+ ioc_state = mpt3sas_base_get_iocstate(ioc, 0);
+ dhsprintk(ioc, printk(MPT3SAS_FMT %s: ioc_state(0x%08x)\n,
+ ioc-name, __func__, ioc_state));
+
+ if (((ioc_state MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_READY) ||
+ (ioc_state MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_OPERATIONAL)
+ return 0;
+
+ if (ioc_state MPI2_DOORBELL_USED) {
+ dhsprintk(ioc, printk(MPT3SAS_FMT
+ unexpected doorbell active!\n, ioc-name));
+ goto issue_diag_reset;
+ }
+
+ if ((ioc_state MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_FAULT) {
+ mpt3sas_base_fault_info(ioc, ioc_state
+ MPI2_DOORBELL_DATA_MASK);
+ goto issue_diag_reset;
+ }
+
+ ioc_state = _base_wait_on_iocstate(ioc, MPI2_IOC_STATE_READY,
+ timeout, sleep_flag);
+ if (ioc_state) {
+ dfailprintk(ioc, printk(MPT3SAS_FMT
+ %s: failed going to ready state (ioc_state=0x%x)\n,
+ ioc-name, __func__, ioc_state));
+ return -EFAULT;
+ }
+
+ issue_diag_reset:
+ rc = _base_diag_reset(ioc, sleep_flag);
+ return rc;
+}
+
+/**
* _base_get_ioc_facts - obtain ioc facts reply and save in ioc
* @ioc: per adapter object
* @sleep_flag: CAN_SLEEP or NO_SLEEP
@@ -3728,6 +3786,13 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc, int
sleep_flag)
dinitprintk(ioc, pr_info(MPT3SAS_FMT %s\n, ioc-name,
__func__));
+ r = _base_wait_for_iocstate(ioc, 10, sleep_flag);
+ if (r) {
+ dfailprintk(ioc, printk(MPT3SAS_FMT
+ %s: failed getting to correct state\n,
+ ioc-name, __func__));
+ return r;
+ }
mpi_reply_sz = sizeof(Mpi2IOCFactsReply_t);
mpi_request_sz = sizeof(Mpi2IOCFactsRequest_t);
memset(mpi_request, 0, mpi_request_sz);
--
Regards,
Sreekanth
--
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: optimal io size / custom alignment
On 17 June 2015 at 05:28, Martin K. Petersen martin.peter...@oracle.com wrote: There are plenty of SSDs that report 4K physical sectors, fwiw. Oh didn't know that. Wonder if it's yet another garbage info. Though 4k is often a nice value to make use of. We gave up on USB-SATA bridges long ago. Their designers appear to have a pretty comprehensive misunderstanding of both the ATA and SCSI protocols. Aren't there tons of thumb drives make use of it anyway? Tom I just feel like the kernel shouldn't bind values from totally Tom different source (raid stripe vs vpd limit) to the same variable. RAID devices communicate the stripe width through the Block Limits VPD. No I put it in the wrong way. What I meant was sd vs md. For example, couldn't the scsi disk driver bind the value it reads from the VPD to another variable instead of optimal i/o size, so that this value would be exclusively for RAID (and other virtual devices)? Is it even necessary for it to report? Because it seems only to make this variable ambiguous. If it HAS TO BE ambiguous, I see no reason why fdisk should use it to derive the alignment. It should simply let the users do their judgement and provide a way for them to adjust 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 1/1] [PATCH] block: Add blk_max_rw_sectors limit
On Tue, Jun 16, 2015 at 06:46:14PM -0400, Martin K. Petersen wrote: Brian, I only have minor nits wrt. your patch since you did what I asked. However, now that I'm less jet lagged and blurry eyed I wonder if the tweak below wouldn't suffice? This would be my preferred version as well. -- 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 1/1] [PATCH] block: Add blk_max_rw_sectors limit
On 06/16/2015 05:46 PM, Martin K. Petersen wrote: Brian, I only have minor nits wrt. your patch since you did what I asked. However, now that I'm less jet lagged and blurry eyed I wonder if the tweak below wouldn't suffice? Works for me. Thanks! Tested-by: Brian King brk...@linux.vnet.ibm.com -Brian -- Brian King Power Linux I/O IBM Linux Technology Center -- 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 v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org wrote: On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote: On Sun, Jun 7, 2015 at 10:32 AM, yga...@codeaurora.org wrote: 2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: [...] If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens always), then the calling to of_platform_populate() which is added, guarantees that ufs-qcom probe will be called and finish, before ufshcd_pltfrm probe continues. so ufs_variant device is always there, and ready. I think it means we are safe - since either way, we make sure ufs-qcom probe will be called and finish before dealing with ufs_variant device in ufshcd_pltfrm probe. This is due to the fact that you have 2 platform drivers. You should only have 1 (and 1 node). If you really think you need 2, then you should do like many other common *HCIs do and make the base UFS driver a set of library functions that drivers can use or call. Look at EHCI, AHCI, SDHCI, etc. for inspiration. Hi Rob, We did look at SDHCI and decided to go with this design due to its simplicity and lack of library functions. Yaniv described the proper flow of probing and, as we understand things, it is guaranteed to work as designed. Furthermore, the design of having a subcore in the dts is used in the Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example - both dwc3-msm and dwc3-exynox invoke the probing function in core.c (i.e. the shared underlying Synopsys USB dwc3 core) by calling of_platform_populate(). That binding has the same problem. Please don't propagate that. There is no point in a sub-node in this case. Do you see a benefit in the SDHCi implementation? Yes, it does not let the kernel driver design dictate the hardware description. Rob Hi Rob, We appear to be having a philosophical disagreement on the practicality of designing the ufshcd variant's implementation - in other words, we disagree on the proper design pattern to follow here. If I understand correctly, you are concerned with a design pattern wherein a generic implementation is wrapped - at the device-tree level - in a variant implementation. The main reason for your concern is that you don't want the kernel driver design dictate the hardware description. We considered this point when we suggested our implementation (both before and after you raised it) and reached the conclusion that - while an important consideration - it should not be the prevailing one. I believe that you will agree once you read the reasoning. What guided us was the following: 1. Keep our change minimal. 2. Keep our patch in line with known design patterns in the kernel. 3. Have our patch extend the existing solution rather than reinvent it. It is the 3rd point that is most important to this discussion, since UFS has already been deployed by various vendors and is used by OEM. Changing ufshcd to a set of library functions that would be called by variants would necessarily introduce a significant change to the code flow in many places and would pose a backward compatibility issue. By using the tried and tested pattern of subnodes in the dts we were able to keep the change simple, succinct, understandable, maintainable and backward compatible. In fact, the entire logic tying of the generic implementation to the variant takes ~20 lines of code - both short and elegant. The DWC3 binding does this and nothing else that I'm aware of. This hardly makes for a common pattern. If you really want to split this to 2 devices, you can create platform devices without having a DT node. If you want to convince me this is the right approach for the binding then you need to convince me the h/w is actually split this way and there is functionality separate from the licensed IP. Rob -- 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 v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org wrote: On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote: On Sun, Jun 7, 2015 at 10:32 AM, yga...@codeaurora.org wrote: 2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: [...] If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens always), then the calling to of_platform_populate() which is added, guarantees that ufs-qcom probe will be called and finish, before ufshcd_pltfrm probe continues. so ufs_variant device is always there, and ready. I think it means we are safe - since either way, we make sure ufs-qcom probe will be called and finish before dealing with ufs_variant device in ufshcd_pltfrm probe. This is due to the fact that you have 2 platform drivers. You should only have 1 (and 1 node). If you really think you need 2, then you should do like many other common *HCIs do and make the base UFS driver a set of library functions that drivers can use or call. Look at EHCI, AHCI, SDHCI, etc. for inspiration. Hi Rob, We did look at SDHCI and decided to go with this design due to its simplicity and lack of library functions. Yaniv described the proper flow of probing and, as we understand things, it is guaranteed to work as designed. Furthermore, the design of having a subcore in the dts is used in the Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example - both dwc3-msm and dwc3-exynox invoke the probing function in core.c (i.e. the shared underlying Synopsys USB dwc3 core) by calling of_platform_populate(). That binding has the same problem. Please don't propagate that. There is no point in a sub-node in this case. Do you see a benefit in the SDHCi implementation? Yes, it does not let the kernel driver design dictate the hardware description. Rob Hi Rob, We appear to be having a philosophical disagreement on the practicality of designing the ufshcd variant's implementation - in other words, we disagree on the proper design pattern to follow here. If I understand correctly, you are concerned with a design pattern wherein a generic implementation is wrapped - at the device-tree level - in a variant implementation. The main reason for your concern is that you don't want the kernel driver design dictate the hardware description. We considered this point when we suggested our implementation (both before and after you raised it) and reached the conclusion that - while an important consideration - it should not be the prevailing one. I believe that you will agree once you read the reasoning. What guided us was the following: 1. Keep our change minimal. 2. Keep our patch in line with known design patterns in the kernel. 3. Have our patch extend the existing solution rather than reinvent it. It is the 3rd point that is most important to this discussion, since UFS has already been deployed by various vendors and is used by OEM. Changing ufshcd to a set of library functions that would be called by variants would necessarily introduce a significant change to the code flow in many places and would pose a backward compatibility issue. By using the tried and tested pattern of subnodes in the dts we were able to keep the change simple, succinct, understandable, maintainable and backward compatible. In fact, the entire logic tying of the generic implementation to the variant takes ~20 lines of code - both short and elegant. The DWC3 binding does this and nothing else that I'm aware of. This hardly makes for a common pattern. If you really want to split this to 2 devices, you can create platform devices without having a DT node. If you want to convince me this is the right approach for the binding then you need to convince me the h/w is actually split this way and there is functionality separate from the licensed IP. Rob I don't understand the challenge that you just posed. It is clear from our implementation that there is the standard and variants thereof. I know this to be a fact on the processors that we are working on. Furthermore, although I didn't check each and every result in the search, of_platform_populate is used in more locations than dwc3 and at least a few of them seem have be using the same paradigm as ours (http://lxr.free-electrons.com/ident?i=of_platform_populate). QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- 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 v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick d...@codeaurora.org wrote: On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org wrote: On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote: On Sun, Jun 7, 2015 at 10:32 AM, yga...@codeaurora.org wrote: 2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: [...] If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens always), then the calling to of_platform_populate() which is added, guarantees that ufs-qcom probe will be called and finish, before ufshcd_pltfrm probe continues. so ufs_variant device is always there, and ready. I think it means we are safe - since either way, we make sure ufs-qcom probe will be called and finish before dealing with ufs_variant device in ufshcd_pltfrm probe. This is due to the fact that you have 2 platform drivers. You should only have 1 (and 1 node). If you really think you need 2, then you should do like many other common *HCIs do and make the base UFS driver a set of library functions that drivers can use or call. Look at EHCI, AHCI, SDHCI, etc. for inspiration. Hi Rob, We did look at SDHCI and decided to go with this design due to its simplicity and lack of library functions. Yaniv described the proper flow of probing and, as we understand things, it is guaranteed to work as designed. Furthermore, the design of having a subcore in the dts is used in the Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example - both dwc3-msm and dwc3-exynox invoke the probing function in core.c (i.e. the shared underlying Synopsys USB dwc3 core) by calling of_platform_populate(). That binding has the same problem. Please don't propagate that. There is no point in a sub-node in this case. Do you see a benefit in the SDHCi implementation? Yes, it does not let the kernel driver design dictate the hardware description. Rob Hi Rob, We appear to be having a philosophical disagreement on the practicality of designing the ufshcd variant's implementation - in other words, we disagree on the proper design pattern to follow here. If I understand correctly, you are concerned with a design pattern wherein a generic implementation is wrapped - at the device-tree level - in a variant implementation. The main reason for your concern is that you don't want the kernel driver design dictate the hardware description. We considered this point when we suggested our implementation (both before and after you raised it) and reached the conclusion that - while an important consideration - it should not be the prevailing one. I believe that you will agree once you read the reasoning. What guided us was the following: 1. Keep our change minimal. 2. Keep our patch in line with known design patterns in the kernel. 3. Have our patch extend the existing solution rather than reinvent it. It is the 3rd point that is most important to this discussion, since UFS has already been deployed by various vendors and is used by OEM. Changing ufshcd to a set of library functions that would be called by variants would necessarily introduce a significant change to the code flow in many places and would pose a backward compatibility issue. By using the tried and tested pattern of subnodes in the dts we were able to keep the change simple, succinct, understandable, maintainable and backward compatible. In fact, the entire logic tying of the generic implementation to the variant takes ~20 lines of code - both short and elegant. The DWC3 binding does this and nothing else that I'm aware of. This hardly makes for a common pattern. If you really want to split this to 2 devices, you can create platform devices without having a DT node. If you want to convince me this is the right approach for the binding then you need to convince me the h/w is actually split this way and there is functionality separate from the licensed IP. Rob I don't understand the challenge that you just posed. It is clear from our implementation that there is the standard and variants thereof. I know this to be a fact on the processors that we are working on. Furthermore, although I didn't check each and every result in the search, of_platform_populate is used in more locations than dwc3 and at least a few of them seem have be using the same paradigm as ours (http://lxr.free-electrons.com/ident?i=of_platform_populate). You can ignore everything under arch/ as those are root level calls. Most of the rest of the list are devices which have multiple unrelated functions such as PMICs or system controllers which are perfectly valid uses of of_platform_populate. That leaves at most 10 examples that may or may not have good bindings. 10 out of hundreds of drivers in the kernel hardly makes for a pattern to follow. Let me be perfectly clear on the binding as is: NAK. I'm done replying until you propose something different. Rob -- To unsubscribe from
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Wed, 2015-06-17 at 14:21 +, Dov Levenglick wrote: Hi James, Rob raises a point that we don't agree with. On the other hand, we are not capable of convincing him in the validity of our approach - we are at an impasse. I would like to point out that our approach was reviewed by Paul and Mita (external reviewers) and neither of them had the objection that Rob has. I urge you to go over this thread, where both sides raised points and argued their cases. We are going to need your call on this so that we can move forward. The dispute is about device tree bindings. While I could override a NAK in the subsystem I maintain, I'm not going to when it comes from the maintainer of the device tree subsystem on a subject that's his province of expertise, not mine. Please agree on what the bindings should look like and then resubmit the driver. James Thanks, Dov On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick d...@codeaurora.org wrote: On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org wrote: On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote: On Sun, Jun 7, 2015 at 10:32 AM, yga...@codeaurora.org wrote: 2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: [...] If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens always), then the calling to of_platform_populate() which is added, guarantees that ufs-qcom probe will be called and finish, before ufshcd_pltfrm probe continues. so ufs_variant device is always there, and ready. I think it means we are safe - since either way, we make sure ufs-qcom probe will be called and finish before dealing with ufs_variant device in ufshcd_pltfrm probe. This is due to the fact that you have 2 platform drivers. You should only have 1 (and 1 node). If you really think you need 2, then you should do like many other common *HCIs do and make the base UFS driver a set of library functions that drivers can use or call. Look at EHCI, AHCI, SDHCI, etc. for inspiration. Hi Rob, We did look at SDHCI and decided to go with this design due to its simplicity and lack of library functions. Yaniv described the proper flow of probing and, as we understand things, it is guaranteed to work as designed. Furthermore, the design of having a subcore in the dts is used in the Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example - both dwc3-msm and dwc3-exynox invoke the probing function in core.c (i.e. the shared underlying Synopsys USB dwc3 core) by calling of_platform_populate(). That binding has the same problem. Please don't propagate that. There is no point in a sub-node in this case. Do you see a benefit in the SDHCi implementation? Yes, it does not let the kernel driver design dictate the hardware description. Rob Hi Rob, We appear to be having a philosophical disagreement on the practicality of designing the ufshcd variant's implementation - in other words, we disagree on the proper design pattern to follow here. If I understand correctly, you are concerned with a design pattern wherein a generic implementation is wrapped - at the device-tree level - in a variant implementation. The main reason for your concern is that you don't want the kernel driver design dictate the hardware description. We considered this point when we suggested our implementation (both before and after you raised it) and reached the conclusion that - while an important consideration - it should not be the prevailing one. I believe that you will agree once you read the reasoning. What guided us was the following: 1. Keep our change minimal. 2. Keep our patch in line with known design patterns in the kernel. 3. Have our patch extend the existing solution rather than reinvent it. It is the 3rd point that is most important to this discussion, since UFS has already been deployed by various vendors and is used by OEM. Changing ufshcd to a set of library functions that would be called by variants would necessarily introduce a significant change to the code flow in many places and would pose a backward compatibility issue. By using the tried and tested pattern of subnodes in the dts we were able to keep the change simple, succinct, understandable, maintainable and backward compatible. In fact, the entire logic tying of the generic implementation to the variant takes ~20 lines of code - both short and elegant. The DWC3 binding does this and nothing else that I'm aware of. This hardly makes for a common pattern. If you really want to split this to 2 devices, you can create platform devices without having a DT node. If you want to convince me this is the right approach for the binding then you need to convince me the h/w is actually split this way and there is functionality separate from the licensed IP. Rob
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
Hi James, Rob raises a point that we don't agree with. On the other hand, we are not capable of convincing him in the validity of our approach - we are at an impasse. I would like to point out that our approach was reviewed by Paul and Mita (external reviewers) and neither of them had the objection that Rob has. I urge you to go over this thread, where both sides raised points and argued their cases. We are going to need your call on this so that we can move forward. Thanks, Dov On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick d...@codeaurora.org wrote: On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org wrote: On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote: On Sun, Jun 7, 2015 at 10:32 AM, yga...@codeaurora.org wrote: 2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: [...] If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens always), then the calling to of_platform_populate() which is added, guarantees that ufs-qcom probe will be called and finish, before ufshcd_pltfrm probe continues. so ufs_variant device is always there, and ready. I think it means we are safe - since either way, we make sure ufs-qcom probe will be called and finish before dealing with ufs_variant device in ufshcd_pltfrm probe. This is due to the fact that you have 2 platform drivers. You should only have 1 (and 1 node). If you really think you need 2, then you should do like many other common *HCIs do and make the base UFS driver a set of library functions that drivers can use or call. Look at EHCI, AHCI, SDHCI, etc. for inspiration. Hi Rob, We did look at SDHCI and decided to go with this design due to its simplicity and lack of library functions. Yaniv described the proper flow of probing and, as we understand things, it is guaranteed to work as designed. Furthermore, the design of having a subcore in the dts is used in the Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example - both dwc3-msm and dwc3-exynox invoke the probing function in core.c (i.e. the shared underlying Synopsys USB dwc3 core) by calling of_platform_populate(). That binding has the same problem. Please don't propagate that. There is no point in a sub-node in this case. Do you see a benefit in the SDHCi implementation? Yes, it does not let the kernel driver design dictate the hardware description. Rob Hi Rob, We appear to be having a philosophical disagreement on the practicality of designing the ufshcd variant's implementation - in other words, we disagree on the proper design pattern to follow here. If I understand correctly, you are concerned with a design pattern wherein a generic implementation is wrapped - at the device-tree level - in a variant implementation. The main reason for your concern is that you don't want the kernel driver design dictate the hardware description. We considered this point when we suggested our implementation (both before and after you raised it) and reached the conclusion that - while an important consideration - it should not be the prevailing one. I believe that you will agree once you read the reasoning. What guided us was the following: 1. Keep our change minimal. 2. Keep our patch in line with known design patterns in the kernel. 3. Have our patch extend the existing solution rather than reinvent it. It is the 3rd point that is most important to this discussion, since UFS has already been deployed by various vendors and is used by OEM. Changing ufshcd to a set of library functions that would be called by variants would necessarily introduce a significant change to the code flow in many places and would pose a backward compatibility issue. By using the tried and tested pattern of subnodes in the dts we were able to keep the change simple, succinct, understandable, maintainable and backward compatible. In fact, the entire logic tying of the generic implementation to the variant takes ~20 lines of code - both short and elegant. The DWC3 binding does this and nothing else that I'm aware of. This hardly makes for a common pattern. If you really want to split this to 2 devices, you can create platform devices without having a DT node. If you want to convince me this is the right approach for the binding then you need to convince me the h/w is actually split this way and there is functionality separate from the licensed IP. Rob I don't understand the challenge that you just posed. It is clear from our implementation that there is the standard and variants thereof. I know this to be a fact on the processors that we are working on. Furthermore, although I didn't check each and every result in the search, of_platform_populate is used in more locations than dwc3 and at least a few of them seem have be using the same paradigm as ours (http://lxr.free-electrons.com/ident?i=of_platform_populate). You can ignore everything under
Re: [PATCH] iSCSI: let session recovery_tmo sysfs writes persist across recovery
On 06/16/2015 06:07 PM, Chris Leech wrote: The iSCSI session recovery_tmo setting is writeable in sysfs, but it's also set every time a connection is established when parameters are set from iscsid over netlink. That results in the timeout being reset to the default value after every recovery. The DM multipath tools want to use the sysfs interface to lower the default timeout when there are multiple paths to fail over. It has caused confusion that we have a writeable sysfs value that seem to keep resetting itself. This patch adds an in-kernel flag that gets set once a sysfs write occurs, and then ignores netlink parameter setting once it's been modified via the sysfs interface. My thinking here is that the sysfs interface is much simpler for external tools to influence the session timeout, but if we're going to allow it to be modified directly we should ensure that setting is maintained. What happened? Why didn't you make it more generic so all future iscsi sysfs settings work the same way like when I reviewed it in the bz? Did it get too messy when we only have the one writeable attr? -- 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 v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
On Wed, 2015-06-17 at 14:21 +, Dov Levenglick wrote: Hi James, Rob raises a point that we don't agree with. On the other hand, we are not capable of convincing him in the validity of our approach - we are at an impasse. I would like to point out that our approach was reviewed by Paul and Mita (external reviewers) and neither of them had the objection that Rob has. I urge you to go over this thread, where both sides raised points and argued their cases. We are going to need your call on this so that we can move forward. The dispute is about device tree bindings. While I could override a NAK in the subsystem I maintain, I'm not going to when it comes from the maintainer of the device tree subsystem on a subject that's his province of expertise, not mine. Please agree on what the bindings should look like and then resubmit the driver. James Hi James Rob, Until this point I thought that this was a disagreement within the confines of the scsi list. I was not aware of Rob's position as a maintainer of the device tree subsystem. We will take this offline with Rob and come back with a solution that works for everyone. Thanks, Dov Thanks, Dov On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick d...@codeaurora.org wrote: On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org wrote: On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote: On Sun, Jun 7, 2015 at 10:32 AM, yga...@codeaurora.org wrote: 2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: [...] If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens always), then the calling to of_platform_populate() which is added, guarantees that ufs-qcom probe will be called and finish, before ufshcd_pltfrm probe continues. so ufs_variant device is always there, and ready. I think it means we are safe - since either way, we make sure ufs-qcom probe will be called and finish before dealing with ufs_variant device in ufshcd_pltfrm probe. This is due to the fact that you have 2 platform drivers. You should only have 1 (and 1 node). If you really think you need 2, then you should do like many other common *HCIs do and make the base UFS driver a set of library functions that drivers can use or call. Look at EHCI, AHCI, SDHCI, etc. for inspiration. Hi Rob, We did look at SDHCI and decided to go with this design due to its simplicity and lack of library functions. Yaniv described the proper flow of probing and, as we understand things, it is guaranteed to work as designed. Furthermore, the design of having a subcore in the dts is used in the Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example - both dwc3-msm and dwc3-exynox invoke the probing function in core.c (i.e. the shared underlying Synopsys USB dwc3 core) by calling of_platform_populate(). That binding has the same problem. Please don't propagate that. There is no point in a sub-node in this case. Do you see a benefit in the SDHCi implementation? Yes, it does not let the kernel driver design dictate the hardware description. Rob Hi Rob, We appear to be having a philosophical disagreement on the practicality of designing the ufshcd variant's implementation - in other words, we disagree on the proper design pattern to follow here. If I understand correctly, you are concerned with a design pattern wherein a generic implementation is wrapped - at the device-tree level - in a variant implementation. The main reason for your concern is that you don't want the kernel driver design dictate the hardware description. We considered this point when we suggested our implementation (both before and after you raised it) and reached the conclusion that - while an important consideration - it should not be the prevailing one. I believe that you will agree once you read the reasoning. What guided us was the following: 1. Keep our change minimal. 2. Keep our patch in line with known design patterns in the kernel. 3. Have our patch extend the existing solution rather than reinvent it. It is the 3rd point that is most important to this discussion, since UFS has already been deployed by various vendors and is used by OEM. Changing ufshcd to a set of library functions that would be called by variants would necessarily introduce a significant change to the code flow in many places and would pose a backward compatibility issue. By using the tried and tested pattern of subnodes in the dts we were able to keep the change simple, succinct, understandable, maintainable and backward compatible. In fact, the entire logic tying of the generic implementation to the variant takes ~20 lines of code - both short and elegant. The DWC3 binding does this and nothing else that I'm aware of. This hardly makes for a common
Re: [PATCH 3/6] target: use 'se_dev_entry' when allocating UAs
Hey Hannes,
Apologies for the delayed follow-up on these, one comment below.
On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote:
We need to use 'se_dev_entry' as argument when allocating
UAs, otherwise we'll never see any UAs for an implicit
ALUA state transition triggered from userspace.
Signed-off-by: Hannes Reinecke h...@suse.de
---
drivers/target/target_core_alua.c | 27 ++-
drivers/target/target_core_pr.c| 31 +--
drivers/target/target_core_transport.c | 18 --
drivers/target/target_core_ua.c| 23 +++
drivers/target/target_core_ua.h| 2 +-
5 files changed, 59 insertions(+), 42 deletions(-)
SNIP
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 436e30b..bb28a97 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -125,6 +125,25 @@ static struct t10_pr_registration
*core_scsi3_locate_pr_reg(struct se_device *,
struct se_node_acl *, struct se_session
*);
static void core_scsi3_put_pr_reg(struct t10_pr_registration *);
+static void core_scsi3_pr_ua_allocate(struct se_node_acl *nacl,
+ u32 unpacked_lun, u8 asc, u8 ascq)
+{
+ struct se_dev_entry *deve;
+
+ if (!nacl)
+ return;
+
+ rcu_read_lock();
+ deve = target_nacl_find_deve(nacl, unpacked_lun);
+ if (!deve) {
+ rcu_read_unlock();
+ return;
+ }
+
+ core_scsi3_ua_allocate(deve, asc, ascq);
+ rcu_read_unlock();
+}
+
This should be common for TCM_RESERVATION_CONFLICT case outside of PR
code too.
Any objections for squashing the following into your original patch..?
Thank you,
--nab
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index bb28a97..0bb3292 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -125,25 +125,6 @@ static struct t10_pr_registration
*core_scsi3_locate_pr_reg(struct se_device *,
struct se_node_acl *, struct se_session
*);
static void core_scsi3_put_pr_reg(struct t10_pr_registration *);
-static void core_scsi3_pr_ua_allocate(struct se_node_acl *nacl,
- u32 unpacked_lun, u8 asc, u8 ascq)
-{
- struct se_dev_entry *deve;
-
- if (!nacl)
- return;
-
- rcu_read_lock();
- deve = target_nacl_find_deve(nacl, unpacked_lun);
- if (!deve) {
- rcu_read_unlock();
- return;
- }
-
- core_scsi3_ua_allocate(deve, asc, ascq);
- rcu_read_unlock();
-}
-
static int target_check_scsi2_reservation_conflict(struct se_cmd *cmd)
{
struct se_session *se_sess = cmd-se_sess;
@@ -2216,7 +2197,7 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64
res_key, u64 sa_res_key,
pr_tmpl-registration_list,
pr_reg_list) {
- core_scsi3_pr_ua_allocate(
+ target_ua_allocate_lun(
pr_reg_p-pr_reg_nacl,
pr_reg_p-pr_res_mapped_lun,
0x2A,
@@ -2643,7 +2624,7 @@ core_scsi3_emulate_pro_release(struct se_cmd *cmd, int
type, int scope,
if (pr_reg_p == pr_reg)
continue;
- core_scsi3_pr_ua_allocate(pr_reg_p-pr_reg_nacl,
+ target_ua_allocate_lun(pr_reg_p-pr_reg_nacl,
pr_reg_p-pr_res_mapped_lun,
0x2A, ASCQ_2AH_RESERVATIONS_RELEASED);
}
@@ -2728,7 +2709,7 @@ core_scsi3_emulate_pro_clear(struct se_cmd *cmd, u64
res_key)
*additional sense code set to RESERVATIONS PREEMPTED.
*/
if (!calling_it_nexus)
- core_scsi3_pr_ua_allocate(pr_reg_nacl,
pr_res_mapped_lun,
+ target_ua_allocate_lun(pr_reg_nacl, pr_res_mapped_lun,
0x2A, ASCQ_2AH_RESERVATIONS_PREEMPTED);
}
spin_unlock(pr_tmpl-registration_lock);
@@ -2937,7 +2918,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int
scope, u64 res_key,
NULL, 0);
}
if (!calling_it_nexus)
- core_scsi3_pr_ua_allocate(pr_reg_nacl,
+ target_ua_allocate_lun(pr_reg_nacl,
pr_res_mapped_lun, 0x2A,
ASCQ_2AH_REGISTRATIONS_PREEMPTED);
}
@@ -3043,7 +3024,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int
scope, u64 res_key,
Re: [PATCH 0/6] target: Update UA handling
On 06/17/2015 08:10 AM, Nicholas A. Bellinger wrote: On Thu, 2015-06-11 at 10:01 +0200, Hannes Reinecke wrote: Hi Nic, lio-target is very minimalistic when it comes to generate UAs; primarily they are generated for persistent reservations, but generic changes tend to be ignored. This patchset updates the UA handling and generates UA for internal state changes (REPORTED LUNS DATA CHANGED, INQUIRY DATA CHANGED, and LUN RESET OCCURRED). Funnily enough this triggers some issues with the SCSI stack; I'll be sending out patches for that, too. Hannes Reinecke (6): target_core_alua: Correct UA handling when switching states target: Remove 'ua_nacl' pointer from se_ua structure target: use 'se_dev_entry' when allocating UAs target: Send UA on ALUA target port group change target: Send UA upon LUN RESET tmr completion target: Send UA when changing LUN inventory drivers/target/target_core_alua.c | 56 +- drivers/target/target_core_device.c| 26 +++- drivers/target/target_core_pr.c| 31 +++ drivers/target/target_core_transport.c | 29 ++ drivers/target/target_core_ua.c| 24 ++- drivers/target/target_core_ua.h| 5 ++- include/target/target_core_base.h | 1 - 7 files changed, 121 insertions(+), 51 deletions(-) Applied to target-pending/for-next, with the extra incremental patch for a common target_ua_alloc_lun() caller. Btw, very happy to see REPORTED_LUNS_DATA_HAS_CHANGED support include for v4.2-rc1 code. 8-) Yeah; I needed a quick testbed for my ALUA update, and thought that tcm_loop would fit the bill. As it turned out, not quite. Hence the patches. BTW: the main issue I have with current lio-target is that you can only configure it _after_ the target has been enabled. IE if you want to add another ALUA state you have to create another TPG, and set this to the required ALUA state. But you can modify the TPG allegiance only _after_ the LUN has been created and is visible to the host. Which means that the initiator inevitably sees both states, and it's impossible to have the LUN start off with a different than the default ALUA state. (This is especially important if one would want to test the READ CAPACITY support in ALUA standby state). Would you be okay with changing that? Cheers, Hannes -- Dr. Hannes ReineckezSeries Storage h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- 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] iSCSI: let session recovery_tmo sysfs writes persist across recovery
On Wed, Jun 17, 2015 at 09:33:04AM -0500, Mike Christie wrote: On 06/16/2015 06:07 PM, Chris Leech wrote: The iSCSI session recovery_tmo setting is writeable in sysfs, but it's also set every time a connection is established when parameters are set from iscsid over netlink. That results in the timeout being reset to the default value after every recovery. The DM multipath tools want to use the sysfs interface to lower the default timeout when there are multiple paths to fail over. It has caused confusion that we have a writeable sysfs value that seem to keep resetting itself. This patch adds an in-kernel flag that gets set once a sysfs write occurs, and then ignores netlink parameter setting once it's been modified via the sysfs interface. My thinking here is that the sysfs interface is much simpler for external tools to influence the session timeout, but if we're going to allow it to be modified directly we should ensure that setting is maintained. What happened? Why didn't you make it more generic so all future iscsi sysfs settings work the same way like when I reviewed it in the bz? Did it get too messy when we only have the one writeable attr? I kept the macro declaration, previous revision had that removed, and modified it so that it could be used for future rw attrs in the same manner. It's possible something could be done for the override check in iscsi_set_param, but there's not a lot of code to share here. - Chris -- 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] iSCSI: let session recovery_tmo sysfs writes persist across recovery
On 06/16/2015 06:07 PM, Chris Leech wrote:
The iSCSI session recovery_tmo setting is writeable in sysfs, but it's
also set every time a connection is established when parameters are set
from iscsid over netlink. That results in the timeout being reset to
the default value after every recovery.
The DM multipath tools want to use the sysfs interface to lower the
default timeout when there are multiple paths to fail over. It has
caused confusion that we have a writeable sysfs value that seem to keep
resetting itself.
This patch adds an in-kernel flag that gets set once a sysfs write
occurs, and then ignores netlink parameter setting once it's been
modified via the sysfs interface. My thinking here is that the sysfs
interface is much simpler for external tools to influence the session
timeout, but if we're going to allow it to be modified directly we
should ensure that setting is maintained.
Signed-off-by: Chris Leech cle...@redhat.com
---
drivers/scsi/scsi_transport_iscsi.c | 11 ---
include/scsi/scsi_transport_iscsi.h | 1 +
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_transport_iscsi.c
b/drivers/scsi/scsi_transport_iscsi.c
index 67d43e3..35ef55f 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2040,6 +2040,7 @@ iscsi_alloc_session(struct Scsi_Host *shost, struct
iscsi_transport *transport,
session-transport = transport;
session-creator = -1;
session-recovery_tmo = 120;
+ session-recovery_tmo_sysfs_override = false;
session-state = ISCSI_SESSION_FREE;
INIT_DELAYED_WORK(session-recovery_work, session_recovery_timedout);
INIT_LIST_HEAD(session-sess_list);
@@ -2784,7 +2785,8 @@ iscsi_set_param(struct iscsi_transport *transport,
struct iscsi_uevent *ev)
switch (ev-u.set_param.param) {
case ISCSI_PARAM_SESS_RECOVERY_TMO:
sscanf(data, %d, value);
- session-recovery_tmo = value;
+ if (!session-recovery_tmo_sysfs_override)
+ session-recovery_tmo = value;
break;
default:
err = transport-set_param(conn, ev-u.set_param.param,
@@ -4047,13 +4049,15 @@ store_priv_session_##field(struct device *dev,
\
if ((session-state == ISCSI_SESSION_FREE) || \
(session-state == ISCSI_SESSION_FAILED)) \
return -EBUSY; \
- if (strncmp(buf, off, 3) == 0)\
+ if (strncmp(buf, off, 3) == 0) { \
session-field = -1;\
- else { \
+ session-field##_sysfs_override = true; \
+ } else {\
val = simple_strtoul(buf, cp, 0); \
if (*cp != '\0' *cp != '\n') \
return -EINVAL; \
session-field = val; \
+ session-field##_sysfs_override = true; \
} \
return count; \
}
@@ -4064,6 +4068,7 @@ store_priv_session_##field(struct device *dev,
\
static ISCSI_CLASS_ATTR(priv_sess, field, S_IRUGO | S_IWUSR, \
show_priv_session_##field, \
store_priv_session_##field)
+
iscsi_priv_session_rw_attr(recovery_tmo, %d);
static struct attribute *iscsi_session_attrs[] = {
diff --git a/include/scsi/scsi_transport_iscsi.h
b/include/scsi/scsi_transport_iscsi.h
index 2555ee5..6183d20 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -241,6 +241,7 @@ struct iscsi_cls_session {
/* recovery fields */
int recovery_tmo;
+ bool recovery_tmo_sysfs_override;
struct delayed_work recovery_work;
unsigned int target_id;
Looks ok to me.
Reviewed-by: Mike Christie micha...@cs.wisc.edu
--
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
