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
> Reviewed-by: Vaneet Narang
> ---
> 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
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
[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
Reviewed-by: Vaneet Narang
---
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
[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:
[] ? dump_stack+0x41/0x51
[] ? warn_slowpath_common+0x77/0x90
[] ? warn_slowpath_fmt+0x4c/0x50
[] ? device_del+0x40/0x1c0
[] ? device_unregister+0x1a/0x70
[] ? bsg_unregister_queue+0x5e/0xb0
[] ? __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:
Reported-by: Praveen Murali
Tested-by: Praveen Murali
Signed-off-by: Dan Williams
---
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
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 > > --- > 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: 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: [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
> ---
> 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
--
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 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 >> > wrote: >> >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick >> >> >>> wrote: >> > On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick >> > >> > wrote: >> >>> On Sun, Jun 7, 2015 at 10:32 AM, wrote: >> > 2015-06-05 5:53 GMT+09:00 : >> > >> > [...] >> > >> 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 vario
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 > Thanks, > Dov > > > > On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick > > wrote: > >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick > >>> wrote: > > On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick > > wrote: > >>> On Sun, Jun 7, 2015 at 10:32 AM, wrote: > > 2015-06-05 5:53 GMT+09:00 : > > > > [...] > > > 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 > f
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 > wrote: >>> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick >>> wrote: > On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick > wrote: >>> On Sun, Jun 7, 2015 at 10:32 AM, wrote: > 2015-06-05 5:53 GMT+09:00 : > > [...] > 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 challen
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 wrote: >> On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick >> wrote: On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick wrote: >> On Sun, Jun 7, 2015 at 10:32 AM, wrote: 2015-06-05 5:53 GMT+09:00 : [...] >>> 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
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 -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 > wrote: >>> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick >>> wrote: > On Sun, Jun 7, 2015 at 10:32 AM, wrote: >>> 2015-06-05 5:53 GMT+09:00 : >>> >>> [...] >>> >> 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 2:42 AM, Dov Levenglick wrote: >> On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick >> wrote: On Sun, Jun 7, 2015 at 10:32 AM, wrote: >> 2015-06-05 5:53 GMT+09:00 : >> >> [...] >> > 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 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
> Reviewed-by: Akhilesh Kumar
> ---
> 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
--
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: [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 Reviewed-by: Tomas Henzl 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
[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
Reviewed-by: Akhilesh Kumar
---
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 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: optimal io size / custom alignment
On 17 June 2015 at 05:28, Martin K. Petersen 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
[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
---
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
[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
---
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
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 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
>> ---
>> 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: [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 > --- 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 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 > wrote: >>> On Sun, Jun 7, 2015 at 10:32 AM, wrote: > 2015-06-05 5:53 GMT+09:00 : > > [...] > 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 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
