Re: [PATCH v14 6/9] added support for DesignWare Controller
Hi Joao, 2016-04-13 21:57 GMT+09:00 Joao Pinto <joao.pi...@synopsys.com>: > > Hi Akinobu, > > On 4/13/2016 1:19 PM, Akinobu Mita wrote: >> Hi Joao, >> >> 2016-04-13 18:04 GMT+09:00 Joao Pinto <joao.pi...@synopsys.com>: >> >>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >>> index e3931d0..c780d14 100644 >>> --- a/drivers/scsi/ufs/ufshcd.h >>> +++ b/drivers/scsi/ufs/ufshcd.h >>> @@ -263,6 +263,7 @@ struct ufs_pwr_mode_info { >>> * @suspend: called during host controller PM callback >>> * @resume: called during host controller PM callback >>> * @dbg_register_dump: used to dump controller debug information >>> + * @phy_initialization: used to initialize phys >>> */ >>> struct ufs_hba_variant_ops { >>> const char *name; >>> @@ -284,6 +285,7 @@ struct ufs_hba_variant_ops { >>> int (*suspend)(struct ufs_hba *, enum ufs_pm_op); >>> int (*resume)(struct ufs_hba *, enum ufs_pm_op); >>> void(*dbg_register_dump)(struct ufs_hba *hba); >>> + int (*phy_initialization)(struct ufs_hba *); >>> }; >> >> This vops->phy_initialization is only called from ufs dwc specific >> source files and not called from ufs core. So this should not belong >> to ufs_hba_variant_ops. > > At this time only DWC specific uses the phy_initialization, but can be used by > others in the future when the same necessity comes. But vops->phy_initialization is only called from DWC specific vops->link_startup_notify. So I think there is no point adding new callback to vops. >> You can use hba->priv to put the controller specific data structure >> including phy_initialization and you also need to define vops->init >> callback which allocates and initialize the data structure. Or you can create another vops which have different link_startup_notify because phy_initialization is only called from link_starup_notify. -- 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 v14 6/9] added support for DesignWare Controller
Hi Joao, 2016-04-13 18:04 GMT+09:00 Joao Pinto: > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index e3931d0..c780d14 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -263,6 +263,7 @@ struct ufs_pwr_mode_info { > * @suspend: called during host controller PM callback > * @resume: called during host controller PM callback > * @dbg_register_dump: used to dump controller debug information > + * @phy_initialization: used to initialize phys > */ > struct ufs_hba_variant_ops { > const char *name; > @@ -284,6 +285,7 @@ struct ufs_hba_variant_ops { > int (*suspend)(struct ufs_hba *, enum ufs_pm_op); > int (*resume)(struct ufs_hba *, enum ufs_pm_op); > void(*dbg_register_dump)(struct ufs_hba *hba); > + int (*phy_initialization)(struct ufs_hba *); > }; This vops->phy_initialization is only called from ufs dwc specific source files and not called from ufs core. So this should not belong to ufs_hba_variant_ops. You can use hba->priv to put the controller specific data structure including phy_initialization and you also need to define vops->init callback which allocates and initialize the data structure. -- 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 v7 3/3] add support for DWC UFS Host Controller
Hi Joao, 2016-02-11 21:13 GMT+09:00 Joao Pinto: > +static int ufshcd_dwc_connection_setup(struct ufs_hba *hba) > +{ > + int ret = 0; > + > + /* Local side Configuration */ > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(N_DEVICEID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(N_DEVICEID_VALID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_PEERDEVICEID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_PEERCPORTID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_TRAFFICCLASS), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CPORTFLAGS), 0x6); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CPORTMODE), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 1); > + if (ret) > + goto out; > + > + > + /* Peer side Configuration */ > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(N_DEVICEID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(N_DEVICEID_VALID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_PEERDEVICEID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_PEERCPORTID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_TRAFFICCLASS), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CPORTFLAGS), 0x6); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CPORTMODE), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 1); > + if (ret) > + goto out; > + > +out: > + return ret; > +} This looks a bit redundant. The most part of the functions in this file is doing ufshcd_dme_set() or ufshcd_dme_peer_set(), so should we introduce ufshcd_dme_set_attrs() like below? It will also increase readability. struct ufshcd_dme_attr_val { u32 attr_sel; u32 mib_val; u8 peer; }; int ufshcd_dme_set_attrs(struct ufs_hba *hba, const struct ufshcd_dme_attr_val *v, int n) { for (i = 0; i < n; i++) { int ret = ufshcd_dme_set_attr(hba, v[i].attr_sel, ...); if (ret) return ret; } return 0; } static int ufshcd_dwc_connection_setup(struct ufs_hba *hba) { const struct ufshcd_dme_attr setup_attrs[] = { { UIC_ARG_MIB(T_CONNECTIONSTATE), 0, DME_LOCAL }, { UIC_ARG_MIB(T_DEVICEID), 0, DME_LOCAL }, ... }; return ufshcd_dme_set_attrs(hba, setup_attrs, ARRAY_SIZE(setup_attrs)); } > +static int ufshcd_dwc_setup_tc(struct ufs_hba *hba) > +{ > + int ret = 0; > + > +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI > + dev_info(hba->dev, "Configuring Test Chip 40-bit RMMI"); > + ret = ufshcd_dwc_setup_40bit_rmmi(hba); > + if (ret) { > + dev_err(hba->dev, "Configuration failed"); Please add \n in the end of message. (and there are same issues in this file) > + > +/* Clock Divider Values: Hex equivalent of frequency in MHz */ > +enum clk_div_values { > + UFSHCD_CLK_DIV_62_5 = 0x3e, > + UFSHCD_CLK_DIV_125 = 0x7d, > + UFSHCD_CLK_DIV_200 = 0xc8, > +}; This is used as register value for DWC_UFS_REG_HCLKDIV. So should they have similar prefix (DWC_UFS_REG_HCLKDIV_*)? > diff --git a/drivers/scsi/ufs/unipro.h b/drivers/scsi/ufs/unipro.h ... > +/*Other attributes*/ > +#define VS_MPHYCFGUPDT 0xD085 > +#define VS_DEBUGOMC0xD09E > +#define VS_POWERSTATE 0xD083 Are these vendor specific attributes? If so, please move them to ufshci-dwc.h. -- 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 2/2] add support for DWC UFS Host Controller
Hi Joao, 2016-02-09 4:07 GMT+09:00 Joao Pinto: > diff --git a/drivers/scsi/ufs/ufshcd-dwc.c b/drivers/scsi/ufs/ufshcd-dwc.c > new file mode 100644 > index 000..b12b1a8 > --- /dev/null > +++ b/drivers/scsi/ufs/ufshcd-dwc.c > @@ -0,0 +1,768 @@ > +/* > + * UFS Host driver for Synopsys Designware Core > + * > + * Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com) > + * > + * Authors: Joao Pinto > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include "ufshcd.h" > +#include "unipro.h" > + > +#include "ufshcd-dwc.h" > +#include "ufshci-dwc.h" > + > +/** > + * ufshcd_dwc_program_clk_div() > + * This function programs the clk divider value. This value is needed to > + * provide 1 microsecond tick to unipro layer. > + * @hba: Private Structure pointer > + * @divider_val: clock divider value to be programmed > + * > + */ > +void ufshcd_dwc_program_clk_div(struct ufs_hba *hba, u32 divider_val) > +{ > + ufshcd_writel(hba, divider_val, DWC_UFS_REG_HCLKDIV); > +} > +EXPORT_SYMBOL(ufshcd_dwc_program_clk_div); > + > +/** > + * ufshcd_dwc_link_is_up() > + * Check if link is up > + * @hba: private structure poitner > + * > + * Returns 0 on success, non-zero value on failure > + */ > +int ufshcd_dwc_link_is_up(struct ufs_hba *hba) > +{ > + int dme_result = 0; > + > + ufshcd_dme_get(hba, UIC_ARG_MIB(VS_POWERSTATE), _result); > + > + if (dme_result == UFSHCD_LINK_IS_UP) { > + ufshcd_set_link_active(hba); > + return 0; > + } > + > + return 1; > +} > +EXPORT_SYMBOL(ufshcd_dwc_link_is_up); > + > +/** > + * ufshcd_dwc_connection_setup() > + * This function configures both the local side (host) and the peer side > + * (device) unipro attributes to establish the connection to application/ > + * cport. > + * This function is not required if the hardware is properly configured to > + * have this connection setup on reset. But invoking this function does no > + * harm and should be fine even working with any ufs device. > + * > + * @hba: pointer to drivers private data > + * > + * Returns 0 on success non-zero value on failure > + */ > +int ufshcd_dwc_connection_setup(struct ufs_hba *hba) > +{ > + int ret = 0; > + > + /* Local side Configuration */ > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(N_DEVICEID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(N_DEVICEID_VALID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_PEERDEVICEID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_PEERCPORTID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_TRAFFICCLASS), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CPORTFLAGS), 0x6); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CPORTMODE), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 1); > + if (ret) > + goto out; > + > + > + /* Peer side Configuration */ > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(N_DEVICEID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(N_DEVICEID_VALID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_PEERDEVICEID), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_PEERCPORTID), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_TRAFFICCLASS), 0); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CPORTFLAGS), 0x6); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CPORTMODE), 1); > + if (ret) > + goto out; > + > + ret = ufshcd_dme_peer_set(hba, UIC_ARG_MIB(T_CONNECTIONSTATE), 1); > + if (ret) > + goto out; > + > +out: > + return ret; > +} > + > +/** > + * ufshcd_dwc_setup_20bit_rmmi_lane0() > + * This function configures Synopsys MPHY 20-bit RMMI Lane 0 > + * @hba: Pointer to drivers structure > + * > + * Returns 0 on success or non-zero value on failure > + */ > +int
Re: [PATCH] add driver for DesignWare UFS Host Controller
Hi Joao, 2016-01-28 2:04 GMT+09:00 Joao Pinto: > - NOP_OUT is failing with OCS = 0x7 > > The OCS value is calculated in ufshcd_get_tr_ocs() in ufshcd.c. > I made a dump of the UTRD pointer where we can check the status = 7 ([2]). > > UTRD at: 7007c3e0 > @ [0]:2100 > @0004 [1]:93936a6a > @0008 [2]:0007 > @000c [3]:93936a6a > @0010 [4]:9f317400 > @0014 [5]: > @0018 [6]:00800080 > @001c [7]:01006a6a > > Did anyone have the same problem? Any hints to overcome this issue? I have seen similar problem when using UFS 2.0 controller. The ufs driver currently doesn't set correct command type field in UTP transfer request descriptor (bit 31:28 in DW0) for UFS 2.0 controller. I checked that your DesignWare UFS driver handles it correctly, so please try with that change. -- 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 v3 05/15] scsi: ufs: increase fDeviceInit query response timeout
2015-10-25 22:38 GMT+09:00: >> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi : >>> fDeviceInit query response time for some devices is too long that >>> default >>> query request timeout of 100ms may not be enough. Experiments show that >>> fDeviceInit response sometimes takes 500ms so to be on safer side this >>> change sets the timeout to 600ms. Without this change, we might >>> unnecessarily have to retry fDeviceInit query requests multiple times >>> and >>> each query request timeout prints one error message. >>> >>> Signed-off-by: Subhash Jadavani >>> Signed-off-by: Yaniv Gardi >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 12 +++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index e0b8755..573a8cb 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -58,6 +58,12 @@ >>> #define QUERY_REQ_RETRIES 10 >>> /* Query request timeout */ >>> #define QUERY_REQ_TIMEOUT 30 /* msec */ >>> +/* >>> + * Query request timeout for fDeviceInit flag >>> + * fDeviceInit query response time for some devices is too large that >>> default >>> + * QUERY_REQ_TIMEOUT may not be enough for such devices. >>> + */ >>> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */ >> >> How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms >> instead of conditionally setting timeout depending on ufs flag? > > Your suggestion obviously could work (increasing the QUERY_REQ_TIMEOUT to > 600ms), but in that case we bring extra delay of 570ms to error handling > of query timeout, and in such a case, instead of handling the error after > 30ms we handle it after 600ms, which make the SW hang. > does it make sense ? Compared to default scsi disk timeout (30s), 600ms does not look very long. I was just worried that the code gets complicated if we need to add yet another QUERY_XYZ_REQ_TIMEOUT macros when it turns out that 30ms timeout is not enough for other query requests under specific conditions. But I don't too much care about it for now. >> >>> >>> /* Task management command timeout */ >>> #define TM_CMD_TIMEOUT 100 /* msecs */ >>> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, >>> enum query_opcode opcode, >>> struct ufs_query_req *request = NULL; >>> struct ufs_query_res *response = NULL; >>> int err, index = 0, selector = 0; >>> + int timeout = QUERY_REQ_TIMEOUT; >>> >>> BUG_ON(!hba); >>> >>> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba, >>> enum query_opcode opcode, >>> goto out_unlock; >>> } >>> >>> - err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, >>> QUERY_REQ_TIMEOUT); >>> + if (idn == QUERY_FLAG_IDN_FDEVICEINIT) >>> + timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT; >>> + >>> + err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout); >>> >>> if (err) { >>> dev_err(hba->dev, >>> -- >>> 1.8.5.2 >>> >>> -- >>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >>> member of Code Aurora Forum, hosted by The Linux Foundation >>> -- >>> 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 >> > > -- 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 v3 13/15] scsi: ufs: add missing memory barriers
2015-10-25 23:40 GMT+09:00: >> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi : >>> Performing several writes to UFS host controller registers has >>> no gurrantee of ordering, so we must make sure register writes >>> to setup request list base address etc. are performed before the >>> run/stop register is enabled. >>> In addition, when setting up a task request, we must make sure >>> the updating of descriptors takes places before ringing the >>> doorbell, similarly to setting up a transfer request. >>> >>> Signed-off-by: Gilad Broner >>> Signed-off-by: Yaniv Gardi >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 21 +++-- >>> 1 file changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index fc2a52d..298511a 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg) >>> * 1 UTRLRDY >>> * 2 UTMRLRDY >>> * 3 UCRDY >>> -* 4 HEI >>> -* 5 DEI >>> -* 6-7 reserved >>> +* 4-7 reserved >>> */ >>> - return (((reg) & (0xFF)) >> 1) ^ (0x07); >>> + return ((reg & 0xFF) >> 1) ^ 0x07; >>> } >>> >>> /** >>> @@ -2726,7 +2724,7 @@ out: >>> * To bring UFS host controller to operational state, >>> * 1. Enable required interrupts >>> * 2. Configure interrupt aggregation >>> - * 3. Program UTRL and UTMRL base addres >>> + * 3. Program UTRL and UTMRL base address >>> * 4. Configure run-stop-registers >>> * >>> * Returns 0 on success, non-zero value on failure >>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct >>> ufs_hba *hba) >>> REG_UTP_TASK_REQ_LIST_BASE_H); >>> >>> /* >>> +* Make sure base address and interrupt setup are updated before >>> +* enabling the run/stop registers below. >>> +*/ >>> + wmb(); >>> + >>> + /* >>> * UCRDY, UTMRLDY and UTRLRDY bits must be 1 >>> -* DEI, HEI bits must be 0 >>> */ >>> reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); >>> if (!(ufshcd_get_lists_status(reg))) { >>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba >>> *hba, int lun_id, int task_id, >>> >>> /* send command to the controller */ >>> __set_bit(free_slot, >outstanding_tasks); >>> + >>> + /* Make sure descriptors are ready before ringing the task >>> doorbell */ >>> + wmb(); >>> + >>> ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); >>> + /* Make sure that doorbell is committed immediately */ >>> + wmb(); >> >> Is this wmb() after ringing tm doorbell is needed? > > Well, Mita, in the case of DB register (Request DB and TASK DB), every > write operation to the DB should be barrier, as if not, out of order > writing to this register might cause inconsistency in its value, and thus, > un-handled requests/tasks. I couldn't fully understand why out of order writing to TASK DB register causes inconsistency. In the TASK request completion (ufshcd_tmc_handler), TASK DB register is read before handling finished requests, so it ensures that all write operations for TASK DB have been performed. >> >>> >>> spin_unlock_irqrestore(host->host_lock, flags); >>> >>> -- >>> 1.8.5.2 >>> >>> -- >>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >>> member of Code Aurora Forum, hosted by The Linux Foundation >>> -- >>> 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 >> > > -- 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 v3 05/15] scsi: ufs: increase fDeviceInit query response timeout
2015-09-02 19:13 GMT+09:00 Yaniv Gardi: > fDeviceInit query response time for some devices is too long that default > query request timeout of 100ms may not be enough. Experiments show that > fDeviceInit response sometimes takes 500ms so to be on safer side this > change sets the timeout to 600ms. Without this change, we might > unnecessarily have to retry fDeviceInit query requests multiple times and > each query request timeout prints one error message. > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 12 +++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index e0b8755..573a8cb 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -58,6 +58,12 @@ > #define QUERY_REQ_RETRIES 10 > /* Query request timeout */ > #define QUERY_REQ_TIMEOUT 30 /* msec */ > +/* > + * Query request timeout for fDeviceInit flag > + * fDeviceInit query response time for some devices is too large that default > + * QUERY_REQ_TIMEOUT may not be enough for such devices. > + */ > +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */ How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms instead of conditionally setting timeout depending on ufs flag? > > /* Task management command timeout */ > #define TM_CMD_TIMEOUT 100 /* msecs */ > @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum > query_opcode opcode, > struct ufs_query_req *request = NULL; > struct ufs_query_res *response = NULL; > int err, index = 0, selector = 0; > + int timeout = QUERY_REQ_TIMEOUT; > > BUG_ON(!hba); > > @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba, enum > query_opcode opcode, > goto out_unlock; > } > > - err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, QUERY_REQ_TIMEOUT); > + if (idn == QUERY_FLAG_IDN_FDEVICEINIT) > + timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT; > + > + err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout); > > if (err) { > dev_err(hba->dev, > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of > Code Aurora Forum, hosted by The Linux Foundation > -- > 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 -- 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 v3 13/15] scsi: ufs: add missing memory barriers
2015-09-02 19:13 GMT+09:00 Yaniv Gardi: > Performing several writes to UFS host controller registers has > no gurrantee of ordering, so we must make sure register writes > to setup request list base address etc. are performed before the > run/stop register is enabled. > In addition, when setting up a task request, we must make sure > the updating of descriptors takes places before ringing the > doorbell, similarly to setting up a transfer request. > > Signed-off-by: Gilad Broner > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 21 +++-- > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index fc2a52d..298511a 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg) > * 1 UTRLRDY > * 2 UTMRLRDY > * 3 UCRDY > -* 4 HEI > -* 5 DEI > -* 6-7 reserved > +* 4-7 reserved > */ > - return (((reg) & (0xFF)) >> 1) ^ (0x07); > + return ((reg & 0xFF) >> 1) ^ 0x07; > } > > /** > @@ -2726,7 +2724,7 @@ out: > * To bring UFS host controller to operational state, > * 1. Enable required interrupts > * 2. Configure interrupt aggregation > - * 3. Program UTRL and UTMRL base addres > + * 3. Program UTRL and UTMRL base address > * 4. Configure run-stop-registers > * > * Returns 0 on success, non-zero value on failure > @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct ufs_hba > *hba) > REG_UTP_TASK_REQ_LIST_BASE_H); > > /* > +* Make sure base address and interrupt setup are updated before > +* enabling the run/stop registers below. > +*/ > + wmb(); > + > + /* > * UCRDY, UTMRLDY and UTRLRDY bits must be 1 > -* DEI, HEI bits must be 0 > */ > reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); > if (!(ufshcd_get_lists_status(reg))) { > @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, > int lun_id, int task_id, > > /* send command to the controller */ > __set_bit(free_slot, >outstanding_tasks); > + > + /* Make sure descriptors are ready before ringing the task doorbell */ > + wmb(); > + > ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); > + /* Make sure that doorbell is committed immediately */ > + wmb(); Is this wmb() after ringing tm doorbell is needed? > > spin_unlock_irqrestore(host->host_lock, flags); > > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of > Code Aurora Forum, hosted by The Linux Foundation > -- > 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 -- 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 v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests
2015-10-21 23:57 GMT+09:00 Akinobu Mita <akinobu.m...@gmail.com>: > 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <yga...@codeaurora.org>: >> DME commands such as Hibern8 enter/exit and gear switch generate 2 >> completion interrupts, one for confirmation that command is received >> by local UniPro and 2nd one is the final confirmation after communication >> with remote UniPro. Currently both of these completions are registered >> as interrupt events which is not quite necessary and instead we can >> just wait for the interrupt of 2nd completion, this should reduce >> the number of interrupts and could reduce the unnecessary CPU wakeups >> to handle extra interrupts. >> >> Signed-off-by: Subhash Jadavani <subha...@codeaurora.org> >> Signed-off-by: Yaniv Gardi <yga...@codeaurora.org> >> >> --- >> drivers/scsi/ufs/ufshcd.c | 41 +++-- >> 1 file changed, 27 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index f2d4301..fc2a52d 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct >> uic_command *uic_cmd) >> * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result >> * @hba: per adapter instance >> * @uic_cmd: UIC command >> + * @completion: initialize the completion only if this is set to true >> * >> * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called >> * with mutex held and host_lock locked. >> * Returns 0 only if success. >> */ >> static int >> -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd) >> +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd, >> + bool completion) >> { >> if (!ufshcd_ready_for_uic_cmd(hba)) { >> dev_err(hba->dev, >> @@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct >> uic_command *uic_cmd) >> return -EIO; >> } >> >> - init_completion(_cmd->done); >> + if (completion) >> + init_completion(_cmd->done); >> >> ufshcd_dispatch_uic_cmd(hba, uic_cmd); >> >> @@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct >> uic_command *uic_cmd) >> ufshcd_add_delay_before_dme_cmd(hba); >> >> spin_lock_irqsave(hba->host->host_lock, flags); >> - ret = __ufshcd_send_uic_cmd(hba, uic_cmd); >> + ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true); >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> if (!ret) >> ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); >> @@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, >> struct uic_command *cmd) >> unsigned long flags; >> u8 status; >> int ret; >> + bool reenable_intr = false; >> >> mutex_lock(>uic_cmd_mutex); >> init_completion(_async_done); >> @@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, >> struct uic_command *cmd) >> >> spin_lock_irqsave(hba->host->host_lock, flags); >> hba->uic_async_done = _async_done; >> - ret = __ufshcd_send_uic_cmd(hba, cmd); >> - spin_unlock_irqrestore(hba->host->host_lock, flags); >> - if (ret) { >> - dev_err(hba->dev, >> - "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n", >> - cmd->command, cmd->argument3, ret); >> - goto out; >> + if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) { >> + ufshcd_disable_intr(hba, UIC_COMMAND_COMPL); >> + /* >> +* Make sure UIC command completion interrupt is disabled >> before >> +* issuing UIC command. >> +*/ >> + wmb(); >> + reenable_intr = true; mmiowb() is more suitable here? Please see "ACQUIRES VS I/O ACCESSES" subsection in Documentation/memory-barriers.txt >> } >> - ret = ufshcd_wait_for_uic_cmd(hba, cmd); >> + ret = __ufshcd_send_uic_cmd(hba, cmd, false); >> + spin_unlock_irqrestore(hba->host->host_lock, flags); >> if (ret) { >> dev_err(hba->dev, >>
Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity
2015-09-02 19:13 GMT+09:00 Yaniv Gardi: > A race condition appear to exist between request completion when > scsi_done() is called to end the request and set the tag back to > -1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error > handling which aborts the command and reuses it to request sense > data. Sending the request sense is done with tag which was set to -1 > and so it is invalid. > Assert command tag passed from scsi layer is valid. > > Signed-off-by: Gilad Broner > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 24 ++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 2d3ebca..8860a57 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba *hba, > struct ufs_pa_layer_attr *desired_pwr_mode); > static int ufshcd_change_power_mode(struct ufs_hba *hba, > struct ufs_pa_layer_attr *pwr_mode); > +static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag) > +{ > + return tag >= 0 && tag < hba->nutrs; > +} > > static inline int ufshcd_enable_irq(struct ufs_hba *hba) > { > @@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, > struct scsi_cmnd *cmd) > hba = shost_priv(host); > > tag = cmd->request->tag; > + if (!ufshcd_valid_tag(hba, tag)) { > + dev_err(hba->dev, > + "%s: invalid command tag %d: cmd=0x%p, > cmd->request=0x%p", > + __func__, tag, cmd, cmd->request); > + BUG(); > + } Is it better to avoid BUG() by WARN_ON() and return if possible? -- 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 v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests
2015-09-02 19:13 GMT+09:00 Yaniv Gardi: > DME commands such as Hibern8 enter/exit and gear switch generate 2 > completion interrupts, one for confirmation that command is received > by local UniPro and 2nd one is the final confirmation after communication > with remote UniPro. Currently both of these completions are registered > as interrupt events which is not quite necessary and instead we can > just wait for the interrupt of 2nd completion, this should reduce > the number of interrupts and could reduce the unnecessary CPU wakeups > to handle extra interrupts. > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 41 +++-- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index f2d4301..fc2a52d 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct > uic_command *uic_cmd) > * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result > * @hba: per adapter instance > * @uic_cmd: UIC command > + * @completion: initialize the completion only if this is set to true > * > * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called > * with mutex held and host_lock locked. > * Returns 0 only if success. > */ > static int > -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd) > +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd, > + bool completion) > { > if (!ufshcd_ready_for_uic_cmd(hba)) { > dev_err(hba->dev, > @@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct > uic_command *uic_cmd) > return -EIO; > } > > - init_completion(_cmd->done); > + if (completion) > + init_completion(_cmd->done); > > ufshcd_dispatch_uic_cmd(hba, uic_cmd); > > @@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct > uic_command *uic_cmd) > ufshcd_add_delay_before_dme_cmd(hba); > > spin_lock_irqsave(hba->host->host_lock, flags); > - ret = __ufshcd_send_uic_cmd(hba, uic_cmd); > + ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true); > spin_unlock_irqrestore(hba->host->host_lock, flags); > if (!ret) > ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd); > @@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, > struct uic_command *cmd) > unsigned long flags; > u8 status; > int ret; > + bool reenable_intr = false; > > mutex_lock(>uic_cmd_mutex); > init_completion(_async_done); > @@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, > struct uic_command *cmd) > > spin_lock_irqsave(hba->host->host_lock, flags); > hba->uic_async_done = _async_done; > - ret = __ufshcd_send_uic_cmd(hba, cmd); > - spin_unlock_irqrestore(hba->host->host_lock, flags); > - if (ret) { > - dev_err(hba->dev, > - "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n", > - cmd->command, cmd->argument3, ret); > - goto out; > + if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) { > + ufshcd_disable_intr(hba, UIC_COMMAND_COMPL); > + /* > +* Make sure UIC command completion interrupt is disabled > before > +* issuing UIC command. > +*/ > + wmb(); > + reenable_intr = true; > } > - ret = ufshcd_wait_for_uic_cmd(hba, cmd); > + ret = __ufshcd_send_uic_cmd(hba, cmd, false); > + spin_unlock_irqrestore(hba->host->host_lock, flags); > if (ret) { > dev_err(hba->dev, > "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n", > @@ -2387,7 +2393,10 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, > struct uic_command *cmd) > } > out: > spin_lock_irqsave(hba->host->host_lock, flags); > + hba->active_uic_cmd = NULL; > hba->uic_async_done = NULL; > + if (reenable_intr) > + ufshcd_enable_intr(hba, UIC_COMMAND_COMPL); > spin_unlock_irqrestore(hba->host->host_lock, flags); > mutex_unlock(>uic_cmd_mutex); > > @@ -3812,16 +3821,20 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 > intr_status) > */ > static irqreturn_t ufshcd_intr(int irq, void *__hba) > { > - u32 intr_status; > + u32 intr_status, enabled_intr_status; > irqreturn_t retval = IRQ_NONE; > struct ufs_hba *hba = __hba; > > spin_lock(hba->host->host_lock); > intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS); > + enabled_intr_status = > +
Re: [PATCH v3 07/15] scsi: ufs: set REQUEST_SENSE command size to 18 bytes
2015-09-02 19:13 GMT+09:00 Yaniv Gardi: > According to UFS device specification REQUEST_SENSE command can > only report back up to 18 bytes of data. > > Signed-off-by: Gilad Broner > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 0e54183..217b5cf 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -42,6 +42,7 @@ > > #include "ufshcd.h" > #include "unipro.h" > +#define UFSHCD_REQ_SENSE_SIZE 18 We usually put a blank line between #include and #define. -- 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 v3 09/15] scsi: ufs: add retries for hibern8 enter
2015-09-02 19:13 GMT+09:00 Yaniv Gardi: > If hibern8 enter command fails then UFS link state may be unknown which > may result into timeout of all the commands issued after failure. > > This change does 2 things (for pre-defined number of retry counts) after > hibern8 enter failure: > 1. Recovers the UFS link to active state > 2. If link is recovered to active state, tries to put the UFS link in >hibern8 enter again until retry count expires. > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 26 -- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 8d5bdf0..5800b08 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -75,6 +75,9 @@ > /* maximum number of link-startup retries */ > #define DME_LINKSTARTUP_RETRIES 3 > > +/* Maximum retries for Hibern8 enter */ > +#define UIC_HIBERN8_ENTER_RETRIES 3 > + > /* maximum number of reset retries before giving up */ > #define MAX_HOST_RESET_RETRIES 5 > > @@ -2389,13 +2392,32 @@ out: > return ret; > } > > -static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba) > +static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba) > { > + int ret; > struct uic_command uic_cmd = {0}; > > uic_cmd.command = UIC_CMD_DME_HIBER_ENTER; > + ret = ufshcd_uic_pwr_ctrl(hba, _cmd); > + > + if (ret) > + dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d", > + __func__, ret); This format string is not terminated with '\n'. (I found several same issues in ufshcd.c) > + > + return ret; > +} > + > +static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba) > +{ > + int ret = 0, retries; > 'ret' will soon be initialized in the loop anyway, so it is unnecessary to initialized here. > - return ufshcd_uic_pwr_ctrl(hba, _cmd); > + for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0; retries--) { > + ret = __ufshcd_uic_hibern8_enter(hba); > + if (!ret || ret == -ENOLINK) > + goto out; > + } > +out: > + return ret; > } > > static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba) > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of > Code Aurora Forum, hosted by The Linux Foundation > -- > 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 -- 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 07/17] scsi: ufs: separate device and host quirks
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > diff --git a/drivers/scsi/ufs/ufs_quirks.c b/drivers/scsi/ufs/ufs_quirks.c > new file mode 100644 > index 000..b649bbf > --- /dev/null > +++ b/drivers/scsi/ufs/ufs_quirks.c > @@ -0,0 +1,101 @@ > +/* > + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include "ufshcd.h" > +#include "ufs_quirks.h" > + > + Single blank line is enough. > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index e5a876c..0803a89 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -39,9 +39,10 @@ > > #include > #include > - > +#include > #include > #include "ufshcd.h" > +#include "ufs_quirks.h" > #include "unipro.h" > #define UFSHCD_REQ_SENSE_SIZE 18 > > @@ -259,6 +260,16 @@ static inline void ufshcd_disable_irq(struct ufs_hba > *hba) > } > } > > +/* replace non-printable or non-ASCII characters with spaces */ > +static inline void ufshcd_remove_non_printable(char *val) > +{ > + if (!val) > + return; > + > + if (*val < 0x20 || *val > 0x7e) > + *val = ' '; > +} > + > /* > * ufshcd_wait_for_register - wait for register value to change > * @hba - per-adapter interface > @@ -2052,6 +2063,82 @@ static inline int ufshcd_read_power_desc(struct > ufs_hba *hba, > return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size); > } > > +int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size) > +{ > + return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size); > +} > +EXPORT_SYMBOL(ufshcd_read_device_desc); > + > +/** > + * ufshcd_read_string_desc - read string descriptor > + * @hba: pointer to adapter instance > + * @desc_index: descriptor index > + * @buf: pointer to buffer where descriptor would be read > + * @size: size of buf > + * @ascii: if true convert from unicode to ascii characters > + * > + * Return 0 in case of success, non-zero otherwise > + */ > +int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index, u8 *buf, > + u32 size, bool ascii) > +{ > + int err = 0; > + > + err = ufshcd_read_desc(hba, > + QUERY_DESC_IDN_STRING, desc_index, buf, size); > + > + if (err) { > + dev_err(hba->dev, "%s: reading String Desc failed after %d > retries. err = %d\n", > + __func__, QUERY_REQ_RETRIES, err); > + goto out; > + } I actually tried this patch and ufshcd_read_desc() always returns -EINVAL due to the following check in the end of ufshcd_read_desc(). if (ret || (buff_len < ufs_query_desc_max_size[desc_id]) || (desc_buf[QUERY_DESC_LENGTH_OFFSET] != ufs_query_desc_max_size[desc_id]) || (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id)) { ... if (!ret) ret = -EINVAL; Could you also include a fix fir ufshcd_read_desc()? -- 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 01/17] scsi: ufs-qcom: add number of lanes per direction
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 1c37a7d..9638553 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -40,6 +40,7 @@ > #include > #include > > +#include > #include "ufshcd.h" > #include "unipro.h" > #define UFSHCD_REQ_SENSE_SIZE 18 > @@ -87,6 +88,8 @@ > /* Interrupt aggregation default timeout, unit: 40us */ > #define INT_AGGR_DEF_TO0x02 > > +#define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2 > + > #define ufshcd_toggle_vreg(_dev, _vreg, _on) \ > ({ \ > int _ret; \ > @@ -5765,6 +5768,21 @@ static struct devfreq_dev_profile ufs_devfreq_profile > = { > .get_dev_status = ufshcd_devfreq_get_dev_status, > }; > > +static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba) > +{ > + struct device *dev = hba->dev; > + int ret; > + > + ret = of_property_read_u32(dev->of_node, "lanes-per-direction", > + >lanes_per_direction); > + if (ret) { > + dev_dbg(hba->dev, > + "%s: failed to read lanes-per-direction, ret=%d\n", > + __func__, ret); > + hba->lanes_per_direction = UFSHCD_DEFAULT_LANES_PER_DIRECTION; > + } > +} > + > /** > * ufshcd_init - Driver initialization routine > * @hba: per-adapter instance > @@ -5788,6 +5806,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem > *mmio_base, unsigned int irq) > hba->mmio_base = mmio_base; > hba->irq = irq; > > + ufshcd_init_lanes_per_dir(hba); > + > err = ufshcd_hba_init(hba); > if (err) > goto out_error; Should this be called in ufshcd_pltfrm_init() and move ufshcd_init_lanes_per_dir() to ufshcd-pltfrm.c? Because all 'of_' stuffs are in ufshcd-pltfrm.c. -- 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 12/17] scsi: ufs: add retry for query descriptors
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > Query commands have 100ms timeout and it may timeout if they are > issued in parallel to ongoing read/write SCSI commands, this change > adds the retry (max: 10) in case command timeouts. > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 48 > --- > 1 file changed, 33 insertions(+), 15 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index a649250..528e46e 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -1906,21 +1906,7 @@ static int ufshcd_query_attr_retry(struct ufs_hba *hba, > return ret; > } > > -/** > - * ufshcd_query_descriptor - API function for sending descriptor requests > - * hba: per-adapter instance > - * opcode: attribute opcode > - * idn: attribute idn to access > - * index: index field > - * selector: selector field > - * desc_buf: the buffer that contains the descriptor > - * buf_len: length parameter passed to the device > - * > - * Returns 0 for success, non-zero in case of failure. > - * The buf_len parameter will contain, on return, the length parameter > - * received on the response. > - */ > -static int ufshcd_query_descriptor(struct ufs_hba *hba, > +static int __ufshcd_query_descriptor(struct ufs_hba *hba, > enum query_opcode opcode, enum desc_idn idn, u8 index, > u8 selector, u8 *desc_buf, int *buf_len) > { > @@ -1985,6 +1971,38 @@ out: > } > > /** > + * ufshcd_query_descriptor - API function for sending descriptor requests > + * hba: per-adapter instance > + * opcode: attribute opcode > + * idn: attribute idn to access > + * index: index field > + * selector: selector field > + * desc_buf: the buffer that contains the descriptor > + * buf_len: length parameter passed to the device > + * > + * Returns 0 for success, non-zero in case of failure. > + * The buf_len parameter will contain, on return, the length parameter > + * received on the response. > + */ > +int ufshcd_query_descriptor(struct ufs_hba *hba, > + enum query_opcode opcode, enum desc_idn idn, u8 index, > + u8 selector, u8 *desc_buf, int *buf_len) > +{ > + int err; > + int retries; > + > + for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) { > + err = __ufshcd_query_descriptor(hba, opcode, idn, index, > + selector, desc_buf, buf_len); > + if (!err || err == -EINVAL) > + break; > + } > + > + return err; > +} > +EXPORT_SYMBOL(ufshcd_query_descriptor); You introduced query flag and attribute APIs for retry version with '_retry' suffix. This function retries but doesn't have '_retry' suffix. Should we have consistent function names for these query APIs? -- 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 06/17] scsi :ufs: verify hba controller hce reg value
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > Sometimes due to hw issues it takes some time to the > host controller register to update. In order to verify the register > has updated, a polling is done until its value is set. > > In addition the functions ufshcd_hba_stop() and > ufshcd_wait_for_register() was updated with an additional input > parameter, indicating the timeout between reads will > be done by sleeping or spinning the cpu. > > Signed-off-by: Raviv Shvili > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 54 > --- > drivers/scsi/ufs/ufshcd.h | 12 +++ > 2 files changed, 35 insertions(+), 31 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 6171da8..e5a876c 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -267,11 +267,12 @@ static inline void ufshcd_disable_irq(struct ufs_hba > *hba) > * @val - wait condition > * @interval_us - polling interval in microsecs > * @timeout_ms - timeout in millisecs > - * > + * @can_sleep - perform sleep or just spin > * Returns -ETIMEDOUT on error, zero on success > */ We usually put a blank line between @argument description and return value description. -- 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 08/17] scsi: ufs: split broken LCC quirk
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > Currently when UFSHCD_BROKEN_LCC quirk is defined, LCC is getting > disabled on both host and device side but there could be a need > where we don't want to disable the LCC on both side hence this change > splits the quirk in 2 parts one for host and one for device. > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 0803a89..411ec17 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3048,6 +3048,11 @@ static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, > bool peer) > return err; > } > > +static inline int ufshcd_disable_host_tx_lcc(struct ufs_hba *hba) > +{ > + return ufshcd_disable_tx_lcc(hba, false); > +} > + > static inline int ufshcd_disable_device_tx_lcc(struct ufs_hba *hba) > { > return ufshcd_disable_tx_lcc(hba, true); > @@ -3095,6 +3100,12 @@ static int ufshcd_link_startup(struct ufs_hba *hba) > goto out; > } > > + if (hba->dev_quirks & UFS_DEVICE_QUIRK_BROKEN_LCC) { > + ret = ufshcd_disable_host_tx_lcc(hba); > + if (ret) > + goto out; > + } > + This dev_quirks is checked just after link startup. But dev_quirk is determined after ufshcd_complete_dev_init(). Is this quirk correctly be applied? > /* Include any host controller configuration via UIC commands */ > ret = ufshcd_vops_link_startup_notify(hba, POST_CHANGE); > if (ret) > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of > Code Aurora Forum, hosted by The Linux Foundation > -- > 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 -- 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/17] scsi: ufs: add option to change default UFS power management level
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > UFS device and link can be put in multiple different low power modes > hence UFS driver supports multiple different low power modes. > By default UFS driver selects the default (optimal) low power mode > (which gives moderate power savings and have relatively less enter > and exit latencies) but we might have to tune this default power > mode for different chipset platforms to meet the low power > requirements/goals. Hence this patch adds option to change default > UFS low power mode (level). > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi ... > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c > index 3196197..1d26e49 100644 > --- a/drivers/scsi/ufs/ufs-qcom.c > +++ b/drivers/scsi/ufs/ufs-qcom.c > @@ -1195,11 +1195,12 @@ static int ufs_qcom_init(struct ufs_hba *hba) > if (res) { > host->dev_ref_clk_ctrl_mmio = > devm_ioremap_resource(dev, res); > - if (IS_ERR(host->dev_ref_clk_ctrl_mmio)) { > - dev_warn(dev, > - "%s: could not map > dev_ref_clk_ctrl_mmio, err %ld\n", > + if (IS_ERR((__force void const *) > + host->dev_ref_clk_ctrl_mmio)) { > + dev_warn(dev, "%s: could not map > dev_ref_clk_ctrl_mmio, err %ld\n", > __func__, > - PTR_ERR(host->dev_ref_clk_ctrl_mmio)); > + PTR_ERR((__force void const *) > + > host->dev_ref_clk_ctrl_mmio)); > host->dev_ref_clk_ctrl_mmio = NULL; > } > host->dev_ref_clk_en_mask = BIT(5); This change looks unrelated. Should this be belong to other patch? -- 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 14/17] scsi: ufs: tune UniPro parameters to optimize hibern8 exit time
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > Optimal values of local UniPro parameters like PA_Hibern8Time & > PA_TActivate can help reduce the hibern8 exit latency. If both host and > device supports UniPro ver1.6 or later, these parameters will be > automatically tuned during link startup itself. But if either host or > device doesn't support UniPro ver 1.6 or later, we have to manually > tune them. But to keep manual tuning logic simple, we will only do > manual tuning if local unipro version doesn't support ver1.6 or later. > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 121 > ++ > drivers/scsi/ufs/ufshcd.h | 1 + > drivers/scsi/ufs/ufshci.h | 2 + > drivers/scsi/ufs/unipro.h | 21 > 4 files changed, 145 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index a8659a9..0938d6c 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -596,6 +596,34 @@ static inline int ufshcd_is_hba_active(struct ufs_hba > *hba) > return (ufshcd_readl(hba, REG_CONTROLLER_ENABLE) & 0x1) ? 0 : 1; > } > > +u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba) > +{ > + /* HCI version 1.0 and 1.1 supports UniPro 1.41 */ > + if ((hba->ufs_version == UFSHCI_VERSION_10) || > + (hba->ufs_version == UFSHCI_VERSION_11)) > + return UFS_UNIPRO_VER_1_41; > + else > + return UFS_UNIPRO_VER_1_6; > +} > +EXPORT_SYMBOL(ufshcd_get_local_unipro_ver); > + > +static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba) > +{ > + /* > +* If both host and device support UniPro ver1.6 or later, PA layer > +* parameters tuning happens during link startup itself. > +* > +* We can manually tune PA layer parameters if either host or device > +* doesn't support UniPro ver 1.6 or later. But to keep manual tuning > +* logic simple, we will only do manual tuning if local unipro version > +* doesn't support ver1.6 or later. > +*/ > + if (ufshcd_get_local_unipro_ver(hba) < UFS_UNIPRO_VER_1_6) > + return true; > + else > + return false; > +} > + > static void ufshcd_ungate_work(struct work_struct *work) > { > int ret; > @@ -4826,6 +4854,98 @@ out: > } > > /** > + * ufshcd_tune_pa_tactivate - Tunes PA_TActivate of local UniPro > + * @hba: per-adapter instance > + * > + * PA_TActivate parameter can be tuned manually if UniPro version is less > than > + * 1.61. PA_TActivate needs to be greater than or equal to peerM-PHY's > + * RX_MIN_ACTIVATETIME_CAPABILITY attribute. This optimal value can help > reduce > + * the hibern8 exit latency. > + * > + * Returns zero on success, non-zero error value on failure. > + */ > +static int ufshcd_tune_pa_tactivate(struct ufs_hba *hba) > +{ > + int ret = 0; > + u32 peer_rx_min_activatetime = 0, tuned_pa_tactivate; > + > + if (!ufshcd_is_unipro_pa_params_tuning_req(hba)) > + return 0; This ufshcd_tune_pa_tactivate() is called only when ufshcd_is_unipro_pa_params_tuning_req() returns true. Is this second check needed? -- 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 16/17] scsi: ufs: add delay before putting UFS rails in low power modes
2015-09-13 23:52 GMT+09:00 Yaniv Gardi: > We put the UFS device in sleep state & UFS link in hibern8 state during > runtime suspaned. After this we put all the UFS rails in low power > modes immediately but it seems some devices may still draw more than > sleep current from UFS rails (especially from VCCQ rail) atleast for > 500us. > To avoid this situation, this change adds 2ms delay before putting > these UFS rails in LPM mode. > > Signed-off-by: Subhash Jadavani > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/ufshcd.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 20b4c0e..786df28 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -5694,6 +5694,15 @@ out: > static void ufshcd_vreg_set_lpm(struct ufs_hba *hba) > { > /* > +* It seems some UFS devices may keep drawing more than sleep current > +* (atleast for 500us) from UFS rails (especially from VCCQ rail). > +* To avoid this situation, add 2ms delay before putting these UFS > +* rails in LPM mode. > +*/ > + if (!ufshcd_is_link_active(hba)) > + usleep_range(2000, 2100); > + Shouldn't we define dev_quirks for this? > + /* > * If UFS device is either in UFS_Sleep turn off VCC rail to save some > * power. > * > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of > Code Aurora Forum, hosted by The Linux Foundation > -- > 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 -- 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 0/8] Fix error message and present UFS variant
Hi Yaniv, 2015-08-23 22:09 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: V3: fixes a few minor issues. V2: fixes a few issues of unnecessary EXPORT_SYMBOL, types of parameters in routine definition, build errors in case CONFIG_PM is not defined and some other minor fixes. I've checked outstanding issues I reported for v1 and v2 are fixed in this version of series. So please feel free to add: Reviewed-by: Akinobu Mita akinobu.m...@gmail.com I still think that we should introduce print_hex_dump_io() or something simpler for dumping __iomem pointer instead of casting 'void __force *'. But it is only used for debug dump function, so I don't too much worry about it. -- 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 7/8] scsi: ufs-qcom: add debug prints for test bus
2015-08-20 22:59 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: +static bool ufs_qcom_testbus_cfg_is_ok(struct ufs_qcom_host *host) +{ + if (host-testbus.select_major = TSTBUS_MAX) { + dev_err(host-hba-dev, + %s: UFS_CFG1[TEST_BUS_SEL} may not equal 0x%05X\n, + __func__, host-testbus.select_major); + return false; + } + + /* +* Not performing check for each individual select_major +* mappings of select_minor, since there is no harm in +* configuring a non-existent select_minor +*/ + if (host-testbus.select_major 0x1F) { select_minor should be checked instead of select_major here? + dev_err(host-hba-dev, + %s: 0x%05X is not a legal testbus option\n, + __func__, host-testbus.select_minor); + return false; + } + + return true; +} -- 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 6/8] scsi: ufs: make the UFS variant a platform device
2015-08-20 22:59 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: @@ -1036,7 +1037,7 @@ void ufs_qcom_clk_scale_notify(struct ufs_hba *hba) * The variant operations configure the necessary controller and PHY * handshake during initialization. */ -static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = { +static struct ufs_hba_variant_ops ufs_hba_qcom_vops = { It's better to keep const. In order to do this, we also need to put const to 'vops' member in struct ufs_hba and the second argument of ufshcd_pltfrm_init(). +static void ufs_qcom_shutdown(struct platform_device *pdev) +{ + ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev)); +} We don't need this function anymore since we have ufshcd_pltfrm_shutdown() now. -static void ufshcd_pltfrm_shutdown(struct platform_device *pdev) +void ufshcd_pltfrm_shutdown(struct platform_device *pdev) { - ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev)); + ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev)); Whitespace is used as code indent. There are same issues in this patch series, so I recommend running checkpatch.pl before sending patches. -- 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 7/8] scsi: ufs-qcom: add debug prints for test bus
2015-08-20 22:59 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: @@ -30,6 +48,14 @@ static void ufs_qcom_get_speed_mode(struct ufs_pa_layer_attr *p, char *result); static int ufs_qcom_get_bus_vote(struct ufs_qcom_host *host, const char *speed_mode); static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote); +static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host); +static void ufs_qcom_dump_regs(struct ufs_hba *hba, int offset, int len, + char *prefix) +{ + print_hex_dump(KERN_ERR, prefix, + len 4 ? DUMP_PREFIX_OFFSET : DUMP_PREFIX_NONE, + 16, 4, hba-mmio_base + offset, len * 4, false); +} This causes a sparse warning as __iomem pointer is passed to print_hex_dump(). -- 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 6/8] scsi: ufs: make the UFS variant a platform device
2015-08-16 19:14 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: +/** + * ufs_qcom_remove - set driver_data of the device to NULL + * @pdev: pointer to platform device handle + * + * Always return 0 + */ +static int ufs_qcom_remove(struct platform_device *pdev) +{ + struct ufs_hba *hba = platform_get_drvdata(pdev); + + pm_runtime_get_sync((pdev)-dev); + ufshcd_remove(hba); + ufshcd_dealloc_host(hba); scsi_host_put (== ufshcd_dealloc_host) is already called in ufshcd_remove(). So we shouldn't call it here again. +static struct platform_driver ufs_qcom_pltform = { + .probe = ufs_qcom_probe, + .remove = ufs_qcom_remove, + .shutdown = ufs_qcom_shutdown, + .driver = { + .name = ufshcd-qcom, + .owner = THIS_MODULE, We don't need to set .owner. Please see commit 37b6fea57b4 (scsi: ufs: drop owner assignment from platform_drivers). diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index 7db9564..20009a9 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -38,20 +38,9 @@ #include linux/of.h #include ufshcd.h +#include ufshcd-pltfrm.h static const struct of_device_id ufs_of_match[]; We can remove this forward declaration as ufs_of_match is removed below. @@ -245,10 +234,11 @@ out: * Returns 0 if successful * Returns non-zero otherwise */ -static int ufshcd_pltfrm_suspend(struct device *dev) +int ufshcd_pltfrm_suspend(struct device *dev) { return ufshcd_system_suspend(dev_get_drvdata(dev)); } +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_suspend); /** * ufshcd_pltfrm_resume - resume power management function @@ -257,23 +247,30 @@ static int ufshcd_pltfrm_suspend(struct device *dev) * Returns 0 if successful * Returns non-zero otherwise */ -static int ufshcd_pltfrm_resume(struct device *dev) +int ufshcd_pltfrm_resume(struct device *dev) { return ufshcd_system_resume(dev_get_drvdata(dev)); } +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_resume); -static int ufshcd_pltfrm_runtime_suspend(struct device *dev) +int ufshcd_pltfrm_runtime_suspend(struct device *dev) { return ufshcd_runtime_suspend(dev_get_drvdata(dev)); } -static int ufshcd_pltfrm_runtime_resume(struct device *dev) +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_suspend); + +int ufshcd_pltfrm_runtime_resume(struct device *dev) { return ufshcd_runtime_resume(dev_get_drvdata(dev)); } -static int ufshcd_pltfrm_runtime_idle(struct device *dev) +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_resume); + +int ufshcd_pltfrm_runtime_idle(struct device *dev) { return ufshcd_runtime_idle(dev_get_drvdata(dev)); } +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_idle); + #else /* !CONFIG_PM */ #define ufshcd_pltfrm_suspend NULL #define ufshcd_pltfrm_resume NULL Since ufshcd_pltfrm_suspend()/resume() and ufshcd_pltfrm_runtime_*() are only defined when CONFIG_PM=y, ufs-qcom.c can't be built when !CONFIG_PM. These #ifdef should be moved to ufshcd-pltfrm.h, or we can export ufshcd_dev_pm_ops instead of the pm functions. @@ -282,18 +279,15 @@ static int ufshcd_pltfrm_runtime_idle(struct device *dev) #define ufshcd_pltfrm_runtime_idle NULL #endif /* CONFIG_PM */ -static void ufshcd_pltfrm_shutdown(struct platform_device *pdev) -{ - ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev)); -} How about exporting this function? ufs-qcom and other variant can use this. +#ifndef UFSHCD_PLTFRM_H_ +#define UFSHCD_PLTFRM_H_ + +#include ufshcd.h + +int ufshcd_pltfrm_init(struct platform_device *pdev, + struct ufs_hba_variant_ops *vops); struct platform_device appears before including linux/platform_device.h. +/* variant specific ops structures */ +#ifdef CONFIG_SCSI_UFS_QCOM +extern struct ufs_hba_variant_ops ufs_hba_qcom_variant; +#endif What is ufs_hba_qcom_variant? I can't find in kernel source and this patch series. -- 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 5/8] scsi: ufs: creates wrapper functions for vops
2015-08-16 19:14 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: In order to simplify the code a set of wrapper functions is created to test and call each of the variant operations. Signed-off-by: Yaniv Gardi yga...@codeaurora.org --- drivers/scsi/ufs/ufs-qcom.c | 3 +- drivers/scsi/ufs/ufshcd.c | 104 +--- drivers/scsi/ufs/ufshcd.h | 98 + 3 files changed, 138 insertions(+), 67 deletions(-) diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index 64c54b7..a6b7e10 100644 --- a/drivers/scsi/ufs/ufs-qcom.c +++ b/drivers/scsi/ufs/ufs-qcom.c @@ -1036,7 +1036,7 @@ void ufs_qcom_clk_scale_notify(struct ufs_hba *hba) * The variant operations configure the necessary controller and PHY * handshake during initialization. */ -static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = { +static struct ufs_hba_variant_ops ufs_hba_qcom_vops = { Why do we need to drop 'const'? .name = qcom, .init = ufs_qcom_init, .exit = ufs_qcom_exit, @@ -1049,6 +1049,5 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = { .suspend= ufs_qcom_suspend, .resume = ufs_qcom_resume, }; -EXPORT_SYMBOL(ufs_hba_qcom_vops); In patch 8, ufs_hba_qcom_vops is exported again... -- 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 8/8] scsi: ufs-qcom: add QUniPro hardware support and power optimizations
2015-08-16 19:14 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: @@ -1208,6 +1510,7 @@ static struct ufs_hba_variant_ops ufs_hba_qcom_vops = { .resume = ufs_qcom_resume, .dbg_register_dump = ufs_qcom_dump_dbg_regs, }; +EXPORT_SYMBOL(ufs_hba_qcom_vops); As I said in the view comment for the patch 5, this shouldn't be exported again. @@ -775,6 +781,12 @@ static inline int ufshcd_vops_resume(struct ufs_hba *hba, enum ufs_pm_op op) return 0; } +static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba) +{ + if (hba-vops hba-vops-dbg_register_dump) + hba-vops-dbg_register_dump(hba); +} + This change should be done in the patch 7? -- 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 8/8] scsi: ufs-qcom: add QUniPro hardware support and power optimizations
Hi Yaniv, 2015-08-16 19:14 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: @@ -708,17 +713,18 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba) return ufshcd_readl(hba, REG_UFS_VERSION); } -static inline void ufshcd_vops_clk_scale_notify(struct ufs_hba *hba) +static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba, + bool up, bool status) { if (hba-vops hba-vops-clk_scale_notify) - return hba-vops-clk_scale_notify(hba); + return hba-vops-clk_scale_notify(hba, up, status); + return 0; } The third argument of clk_scale_notify() vops is PRE_CHANGE or POST_CHANGE. So should it also be converted from bool to enum ufs_notify_change_status like other vops does? -- 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 v2 6/6] scsi: ufs: fix module reference for scsi host
While accessing a UFS device, the module reference count for core driver (ufshcd) is incremented but not incremented for the actual glue driver (ufshcd-pci or ufshcd-pltfrm). Because these drivers allocate scsi hosts with scsi_host_template defined in ufshcd module. So these drivers always can be unloaded. This fixes it by preparing scsi host template which is initialized at module_init() for each ufs glue driver. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd-pci.c| 19 +-- drivers/scsi/ufs/ufshcd-pltfrm.c | 19 +-- drivers/scsi/ufs/ufshcd.c| 19 +++ drivers/scsi/ufs/ufshcd.h| 5 - 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c index d15eaa4..961c4ad 100644 --- a/drivers/scsi/ufs/ufshcd-pci.c +++ b/drivers/scsi/ufs/ufshcd-pci.c @@ -106,6 +106,8 @@ static void ufshcd_pci_remove(struct pci_dev *pdev) ufshcd_remove(hba); } +static struct scsi_host_template ufshcd_pci_host_template; + /** * ufshcd_pci_probe - probe routine of the driver * @pdev: pointer to PCI device handle @@ -136,7 +138,7 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) mmio_base = pcim_iomap_table(pdev)[0]; - err = ufshcd_alloc_host(pdev-dev, hba); + err = ufshcd_alloc_host(pdev-dev, ufshcd_pci_host_template, hba); if (err) { dev_err(pdev-dev, Allocation failed\n); return err; @@ -183,7 +185,20 @@ static struct pci_driver ufshcd_pci_driver = { }, }; -module_pci_driver(ufshcd_pci_driver); +static int __init ufshcd_pci_driver_init(void) +{ + ufshcd_host_template_init(ufshcd_pci_host_template, ufshcd-pci, + THIS_MODULE); + + return pci_register_driver(ufshcd_pci_driver); +} +module_init(ufshcd_pci_driver_init); + +static void __exit ufshcd_pci_driver_exit(void) +{ + pci_unregister_driver(ufshcd_pci_driver); +} +module_exit(ufshcd_pci_driver_exit); MODULE_AUTHOR(Santosh Yaragnavi santosh...@samsung.com); MODULE_AUTHOR(Vinayak Holikatti h.vina...@samsung.com); diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index 12f1246..2a137c2 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -287,6 +287,8 @@ static void ufshcd_pltfrm_shutdown(struct platform_device *pdev) ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev)); } +static struct scsi_host_template ufshcd_pltfrm_host_template; + /** * ufshcd_pltfrm_probe - probe routine of the driver * @pdev: pointer to Platform device handle @@ -315,7 +317,7 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev) goto out; } - err = ufshcd_alloc_host(dev, hba); + err = ufshcd_alloc_host(dev, ufshcd_pltfrm_host_template, hba); if (err) { dev_err(pdev-dev, Allocation failed\n); goto out; @@ -400,7 +402,20 @@ static struct platform_driver ufshcd_pltfrm_driver = { }, }; -module_platform_driver(ufshcd_pltfrm_driver); +static int __init ufshcd_pltfrm_driver_init(void) +{ + ufshcd_host_template_init(ufshcd_pltfrm_host_template, ufshcd-pltfrm, + THIS_MODULE); + + return platform_driver_register(ufshcd_pltfrm_driver); +} +module_init(ufshcd_pltfrm_driver_init); + +static void __exit ufshcd_pltfrm_driver_exit(void) +{ + platform_driver_unregister(ufshcd_pltfrm_driver); +} +module_exit(ufshcd_pltfrm_driver_exit); MODULE_AUTHOR(Santosh Yaragnavi santosh...@samsung.com); MODULE_AUTHOR(Vinayak Holikatti h.vina...@samsung.com); diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index d287207..6fa64bd 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4338,7 +4338,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie) ufshcd_probe_hba(hba); } -static struct scsi_host_template ufshcd_driver_template = { +static const struct scsi_host_template ufshcd_driver_template = { .module = THIS_MODULE, .name = UFSHCD, .proc_name = UFSHCD, @@ -4359,6 +4359,16 @@ static struct scsi_host_template ufshcd_driver_template = { .track_queue_depth = 1, }; +void ufshcd_host_template_init(struct scsi_host_template *sht, const char *name, + struct module *owner) +{ + *sht = ufshcd_driver_template; + sht-name = name; + sht-proc_name = name; + sht-module = owner
[PATCH v2 5/6] scsi: ufs: fix unloading module while runtime suspended
The ufs driver calls scsi_device_get() in ufshcd_set_dev_pwr_mode() in order to avoid manual delete of UFS device W-LUN by holding the reference. But scsi_device_get() has been changed to fail when the LLD module is in the process of being unloaded. So it no longer doesn't work if the module is unloaded while the device is runtime suspended. (i.e. driver_detach - ... pm_runtime_get_sync() ... - ufshcd_runtime_resume - ufshcd_resume - ufshcd_set_dev_pwr_mode - scsi_device_get - try_module_get - return -ENXIO) As the reason for scsi_device_get() is to avoid manual delete of UFS device W-LUN, this acquires shost-scan_mutex lock instead of scsi_device_get() to work around the problem. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@odin.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Akinobu Mita akinobu.m...@gmail.com Cc: Subhash Jadavani subha...@codeaurora.org Cc: Sahitya Tummala stumm...@codeaurora.org Cc: Yaniv Gardi yga...@codeaurora.org Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd.c | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index d425816..d287207 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4792,23 +4792,19 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, struct scsi_sense_hdr sshdr; struct scsi_device *sdp; unsigned long flags; - int ret; + int ret = 0; + mutex_lock(hba-host-scan_mutex); spin_lock_irqsave(hba-host-host_lock, flags); sdp = hba-sdev_ufs_device; - if (sdp) { - ret = scsi_device_get(sdp); - if (!ret !scsi_device_online(sdp)) { - ret = -ENODEV; - scsi_device_put(sdp); - } - } else { + if (!sdp || !scsi_device_online(sdp)) ret = -ENODEV; - } spin_unlock_irqrestore(hba-host-host_lock, flags); - if (ret) + if (ret) { + mutex_unlock(hba-host-scan_mutex); return ret; + } /* * If scsi commands fail, the scsi mid-layer schedules scsi error- @@ -4845,7 +4841,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, if (!ret) hba-curr_dev_pwr_mode = pwr_mode; out: - scsi_device_put(sdp); + mutex_unlock(hba-host-scan_mutex); hba-host-eh_noresume = 0; return ret; } -- 1.9.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 v2 3/6] scsi: ufs: fix unbalanced power.disable_depth after reloading driver
Every time the driver is reloaded, the warning message Unbalanced pm_runtime_enable! is triggered due to unbalanced power.disable_depth. This is because pm_runtime_enable() is called during driver probe but pm_runtime_disable() is missed on driver remove. This also restores the device's runtime PM status to 'suspended' on driver remove as it was set to 'active' during driver probe. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Maya Erez me...@codeaurora.org Cc: Raviv Shvili rshv...@codeaurora.org Cc: Sahitya Tummala stumm...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Rafael J. Wysocki rafael.j.wyso...@intel.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd-pltfrm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index 9beac71..12f1246 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -369,6 +369,10 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev) pm_runtime_get_sync((pdev)-dev); ufshcd_remove(hba); pm_runtime_put_noidle(pdev-dev); + + pm_runtime_disable(pdev-dev); + pm_runtime_set_suspended(pdev-dev); + return 0; } -- 1.9.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 v2 0/6] scsi: ufs: fix several issues caused by driver reloading
This patch set addresses several issues caused by driver reloading in ufs driver. The first version of this series was sent on March 28. This version gained three more fixes. * Changes from v1 - Call pm_runtime_put_noidle() where idle callback shouldn't be called - Prevent IRQ handler accessing already freed hostdata - Fix unloading module while runtime suspended - Fix module reference for scsi host Akinobu Mita (6): scsi: ufs: avoid using hostdata after scsi_host_put() scsi: ufs: fix unbalanced power.usage_count after reloading driver scsi: ufs: fix unbalanced power.disable_depth after reloading driver scsi: ufs: prevent IRQ handler accessing already freed hostdata scsi: ufs: fix unloading module while runtime suspended scsi: ufs: fix module reference for scsi host drivers/scsi/ufs/ufshcd-pci.c| 19 -- drivers/scsi/ufs/ufshcd-pltfrm.c | 24 -- drivers/scsi/ufs/ufshcd.c| 54 +--- drivers/scsi/ufs/ufshcd.h| 5 +++- 4 files changed, 72 insertions(+), 30 deletions(-) -- 1.9.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 v2 2/6] scsi: ufs: fix unbalanced power.usage_count after reloading driver
On driver removal, pm_runtime_get_sync() is called, but pm_runtime_put_*() is missed. So once the driver is reloaded, the device's power.usage_count is unbalanced and the idle callback for the device will never be called. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Maya Erez me...@codeaurora.org Cc: Sahitya Tummala stumm...@codeaurora.org Cc: Rafael J. Wysocki rafael.j.wyso...@intel.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd-pltfrm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index 7db9564..9beac71 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -368,6 +368,7 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev) pm_runtime_get_sync((pdev)-dev); ufshcd_remove(hba); + pm_runtime_put_noidle(pdev-dev); return 0; } -- 1.9.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 v2 4/6] scsi: ufs: prevent IRQ handler accessing already freed hostdata
As UFS driver registers IRQ handler as a shared IRQ, when CONFIG_DEBUG_SHIRQ=y, an extra call will be made while unregistering the IRQ handler. Unfortunately, the extra call will accesses already freed hostdata. This is because devm_request_irq() is used to register IRQ handler so that it will be unregistered automatically on driver remove, but the hostdata has already been freed at this time. This fixes it by explicitly registering/unregistering IRQ handler on driver probe/remove. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@odin.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Hannes Reinecke h...@suse.de Cc: Sahitya Tummala stumm...@codeaurora.org Cc: Yaniv Gardi yga...@codeaurora.org Cc: linux-scsi@vger.kernel.org Cc: linux-ker...@vger.kernel.org --- drivers/scsi/ufs/ufshcd.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index e25f919..d425816 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -5361,6 +5361,7 @@ void ufshcd_remove(struct ufs_hba *hba) scsi_remove_host(hba-host); /* disable interrupts */ ufshcd_disable_intr(hba, hba-intr_mask); + ufshcd_disable_irq(hba); ufshcd_hba_stop(hba); ufshcd_exit_clk_gating(hba); @@ -5611,13 +5612,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) ufshcd_init_clk_gating(hba); /* IRQ registration */ - err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba); - if (err) { - dev_err(hba-dev, request irq failed\n); + err = ufshcd_enable_irq(hba); + if (err) goto exit_gating; - } else { - hba-is_irq_enabled = true; - } /* Enable SCSI tag mapping */ err = scsi_init_shared_tag_map(host, host-can_queue); @@ -5668,9 +5665,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) out_remove_scsi_host: scsi_remove_host(hba-host); exit_gating: + ufshcd_disable_irq(hba); ufshcd_exit_clk_gating(hba); out_disable: - hba-is_irq_enabled = false; ufshcd_hba_exit(hba); out_error: scsi_host_put(host); -- 1.9.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 v2 1/6] scsi: ufs: avoid using hostdata after scsi_host_put()
The hostdata array, which is denoted by 'hba' in ufs driver, should not be accessed after calling scsi_host_put(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Hannes Reinecke h...@suse.de Cc: Sahitya Tummala stumm...@codeaurora.org Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index b0ade73..e25f919 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -5363,12 +5363,12 @@ void ufshcd_remove(struct ufs_hba *hba) ufshcd_disable_intr(hba, hba-intr_mask); ufshcd_hba_stop(hba); - scsi_host_put(hba-host); - ufshcd_exit_clk_gating(hba); if (ufshcd_is_clkscaling_enabled(hba)) devfreq_remove_device(hba-devfreq); ufshcd_hba_exit(hba); + + scsi_host_put(hba-host); } EXPORT_SYMBOL_GPL(ufshcd_remove); @@ -5671,9 +5671,9 @@ exit_gating: ufshcd_exit_clk_gating(hba); out_disable: hba-is_irq_enabled = false; - scsi_host_put(host); ufshcd_hba_exit(hba); out_error: + scsi_host_put(host); return err; } EXPORT_SYMBOL_GPL(ufshcd_init); -- 1.9.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 v3 4/4] scsi: ufs: probe and init of variant driver from the platform device
2015-06-08 1:27 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: static int ufshcd_pltfrm_remove(struct platform_device *pdev) { struct ufs_hba *hba = platform_get_drvdata(pdev); + struct device_node *node = pdev-dev.of_node; + struct device_node *ufs_variant_node; + struct platform_device *ufs_variant_pdev; + + ufs_variant_node = of_get_next_available_child(node, NULL); + + if (!ufs_variant_node) + dev_dbg(pdev-dev, no ufs_variant_node found\n); + else + ufs_variant_pdev = of_find_device_by_node(ufs_variant_node); pm_runtime_get_sync((pdev)-dev); ufshcd_remove(hba); + module_put(ufs_variant_pdev-dev.driver-owner); module_put() should only be called when ufs_variant sub-node exists and hba-vops was found. If ufs-vops == NULL or no ufs_variant sub-node exists, this line causes uninitialized pointer dereference. return 0; } -- 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
2015-06-08 0:32 GMT+09:00 yga...@codeaurora.org: 1) 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. I'm worrying the case ufshcd_pltfrm_probe() is called when ufshcd-pltfrm module is installed but ufs-qcom module is _not_ installed yet, where ufshcd-pltfrm and ufs-qcom are both built as loadable modules. In this case, of_platform_populate() in ufshcd_pltfrm_probe() doesn't invoke ufs-qcom probe, does it? So I suggested using deferred probe infrastructure by returning -EPROBE_DEFER. 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. 2) you are right. the fix added as you suggested. Thanks for fixing it. But a little more work is needed in v3, I'll leave a comment to v3. -- 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
2015-06-05 5:53 GMT+09:00 yga...@codeaurora.org: Hi Yaniv, 2015-06-03 18:37 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: @@ -321,7 +313,22 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev) goto out; } - hba-vops = get_variant_ops(pdev-dev); + err = of_platform_populate(node, NULL, NULL, pdev-dev); + if (err) + dev_err(pdev-dev, + %s: of_platform_populate() failed\n, __func__); + + ufs_variant_node = of_get_next_available_child(node, NULL); + + if (!ufs_variant_node) { + dev_dbg(pdev-dev, failed to find ufs_variant_node child\n); + } else { + ufs_variant_pdev = of_find_device_by_node(ufs_variant_node); + + if (ufs_variant_pdev) + hba-vops = (struct ufs_hba_variant_ops *) + dev_get_drvdata(ufs_variant_pdev-dev); + } I have no strong objection to 'ufs_variant' sub-node. But why can't we simply add an of_device_id to ufs_of_match, like below: static const struct of_device_id ufs_of_match[] = { { .compatible = jedec,ufs-1.1}, #if IS_ENABLED(SCSI_UFS_QCOM) { .compatible = qcom,ufs, .data = ufs_hba_qcom_vops }, #neidf {}, }; and get hba-vops by get_variant_ops()? Hi Mita, thanks for your comments. The whole idea, of having a sub-node which includes all variant specific attributes is to separate the UFS Platform device component, from the need to know qcom or any other future variant. I believe it keeps the code more modular, and clean - meaning - no #ifdef's and no need to include all variant attributes inside the driver DT node. in that case, we simply have a DT node that is compatible to the Jdec standard, and sub-node to include variant info. I hope you agree with this new design, since it provides a good answer to every future variant that will be added, without the need to change the platform file. Thanks for your explanation, I agree with it. I found two problems in the current code, but both can be fixed relatively easily as described below: 1) If ufshcd-pltfrm driver is loaded before ufs-qcom driver, ufshcd_pltfrm_probe() can't find a ufs_variant device. In order to trigger re-probing ufs device when ufs-qcom driver has been loaded, ufshcd_pltfrm_probe() should return -EPROBE_DEFER in case 'ufs_variant' sub-node exists and no hba-vops found. 2) Nothing prevents ufs-qcom module from being unloaded while the variant_ops is referenced by ufshcd-pltfrm. It can be fixed by incrementing module refcount of ufs_variant module by __module_get(ufs_variant_pdev-dev.driver-owener) in ufshcd_pltfrm_probe(), and module_put() in ufshcd_pltfrm_remove() to descrement the refcount. -- 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
Hi Yaniv, 2015-06-03 18:37 GMT+09:00 Yaniv Gardi yga...@codeaurora.org: @@ -321,7 +313,22 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev) goto out; } - hba-vops = get_variant_ops(pdev-dev); + err = of_platform_populate(node, NULL, NULL, pdev-dev); + if (err) + dev_err(pdev-dev, + %s: of_platform_populate() failed\n, __func__); + + ufs_variant_node = of_get_next_available_child(node, NULL); + + if (!ufs_variant_node) { + dev_dbg(pdev-dev, failed to find ufs_variant_node child\n); + } else { + ufs_variant_pdev = of_find_device_by_node(ufs_variant_node); + + if (ufs_variant_pdev) + hba-vops = (struct ufs_hba_variant_ops *) + dev_get_drvdata(ufs_variant_pdev-dev); + } I have no strong objection to 'ufs_variant' sub-node. But why can't we simply add an of_device_id to ufs_of_match, like below: static const struct of_device_id ufs_of_match[] = { { .compatible = jedec,ufs-1.1}, #if IS_ENABLED(SCSI_UFS_QCOM) { .compatible = qcom,ufs, .data = ufs_hba_qcom_vops }, #neidf {}, }; and get hba-vops by get_variant_ops()? There is something similar in drivers/net/ethernet/freescale/fsl_pq_mdio.c -- 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 v6 0/4] scsi: ufs ums-* esp_scsi: fix module reference counting
While accessing a scsi_device, the use count of the underlying LLDD module is incremented. The module reference is retrieved through .module field of struct scsi_host_template. This mapping between scsi_device and underlying LLDD module works well except ufs, unusual usb storage drivers, and sub drivers for esp_scsi. These drivers consist with core driver and actual LLDDs, and scsi_host_template is defined in the core driver. So the actual LLDDs can be unloaded even if the scsi_device is being accessed. This patch series first adds ability to adjust module reference for scsi host by LLDDs and then fixes actual LLDDs by adjusting module reference after scsi host allocation. * v6: - Rebased as v5 doesn't apply cleanly to the latest tree anymore. * v5: - Discard v4 changes and restore to v3. Because v4 shows that moving owner module field from scsi_host_template to Scsi_Host requires a lot of changes and introduces complication to existing drivers which don't have the module reference mismatch issue. - Rebased to the 4.0-rc1 * v4: - Patch series is almost rewritten as module reference field in struct scsi_host_template has been unused anymore. So Acked-by: and Reviewed-by: tags that have been received are deleted. * v3: - Add fix for ESP SCSI drivers * v2: - Pass correct module reference to usb_stor_probe1() instead of touching all ums-* drivers, suggested by Alan Stern Akinobu Mita (4): scsi: add ability to adjust module reference for scsi host scsi: ufs: adjust module reference for scsi host usb: storage: adjust module reference for scsi host scsi: esp_scsi: adjust module reference for scsi host drivers/scsi/am53c974.c | 3 +-- drivers/scsi/esp_scsi.c | 16 +--- drivers/scsi/esp_scsi.h | 11 +++ drivers/scsi/hosts.c | 1 + drivers/scsi/jazz_esp.c | 3 +-- drivers/scsi/mac_esp.c | 3 +-- drivers/scsi/scsi.c | 4 ++-- drivers/scsi/sun3x_esp.c | 3 +-- drivers/scsi/sun_esp.c | 3 +-- drivers/scsi/ufs/ufshcd-pci.c| 1 + drivers/scsi/ufs/ufshcd-pltfrm.c | 1 + drivers/scsi/ufs/ufshcd.c| 1 - drivers/usb/storage/usb.c| 8 +--- drivers/usb/storage/usb.h| 7 +-- include/scsi/scsi_host.h | 1 + 15 files changed, 41 insertions(+), 25 deletions(-) Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Matthew Dharm mdharm-...@one-eyed-alien.net Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Alan Stern st...@rowland.harvard.edu Cc: David S. Miller da...@davemloft.net Cc: Hannes Reinecke h...@suse.de Cc: linux-...@vger.kernel.org Cc: usb-stor...@lists.one-eyed-alien.net Cc: linux-scsi@vger.kernel.org -- 1.9.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 v4 1/4] target: Fix inconsistent address passed to kunmap_atomic() in sbc_dif_copy_prot()
In sbc_dif_copy_prot(), the addresses passed to kunmap_atomic() are inconsistent with the addresses which are mapped by kmap_atomic(). That could be problematic if an SG element has its length larger than PAGE_SIZE as kunmap_atomic() will attempt to unmap different page. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * New patch from v3 drivers/target/target_core_sbc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index edba39f..b765cdd 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1299,13 +1299,14 @@ void sbc_dif_copy_prot(struct se_cmd *cmd, unsigned int sectors, bool read, copied += len; psg_len -= len; + kunmap_atomic(addr - sg-offset - offset); + if (offset = sg-length) { sg = sg_next(sg); offset = 0; } - kunmap_atomic(addr); } - kunmap_atomic(paddr); + kunmap_atomic(paddr - psg-offset); } } EXPORT_SYMBOL(sbc_dif_copy_prot); -- 1.9.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 3/3] scsi: proper state checking and module refcount handling in scsi_device_get
2015-02-02 22:01 GMT+09:00 Christoph Hellwig h...@lst.de: This effectively reverts commits 85b6c7 ([SCSI] sd: fix cache flushing on module removal (and individual device removal) and dc4515ea (scsi: always increment reference count). We now never call scsi_device_get from the shutdown path, and the fact that we started grabbing reference there in commit 85b6c7 turned out turned out to create more problems than it solves, and required workarounds for workarounds for workarounds. Move back to properly checking the device state and carefully handle module refcounting. Signed-off-by: Christoph Hellwig h...@lst.de --- drivers/scsi/scsi.c | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 9b38299..9b7fd0b 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -979,18 +979,24 @@ EXPORT_SYMBOL(scsi_report_opcode); * Description: Gets a reference to the scsi_device and increments the use count * of the underlying LLDD module. You must hold host_lock of the * parent Scsi_Host or already have a reference when calling this. + * + * This will fail if a device is deleted or cancelled, or when the LLD module + * is in the process of being unloaded. */ int scsi_device_get(struct scsi_device *sdev) Hi Christoph, This change broke ufs driver. Because scsi_device_get() can be called while the module is being unloaded with the device runtime suspended. (i.e. driver_detach - ... pm_runtime_get_sync() ... - ufshcd_runtime_resume - ufshcd_resume - ufshcd_set_dev_pwr_mode - scsi_device_get - try_module_get - return -ENXIO) The reason for scsi_device_get() in ufshcd_set_dev_pwr_mode() is to avoid manual delete of UFS device W-LUN by holding the reference to it. So can we acquire shost-scan_mutex lock instead of scsi_device_get()? I tried attached patch and it seems to be working, but I would like to ask your opinion about this change. { - if (sdev-sdev_state == SDEV_DEL) - return -ENXIO; + if (sdev-sdev_state == SDEV_DEL || sdev-sdev_state == SDEV_CANCEL) + goto fail; if (!get_device(sdev-sdev_gendev)) - return -ENXIO; - /* We can fail try_module_get if we're doing SCSI operations -* from module exit (like cache flush) */ - __module_get(sdev-host-hostt-module); - + goto fail; + if (!try_module_get(sdev-host-hostt-module)) + goto fail_put_device; return 0; + +fail_put_device: + put_device(sdev-sdev_gendev); +fail: + return -ENXIO; } EXPORT_SYMBOL(scsi_device_get); -- 1.9.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 From a305a5284cac23adbf7f86b3014cc2e6325c7b88 Mon Sep 17 00:00:00 2001 From: Akinobu Mita akinobu.m...@gmail.com Date: Wed, 29 Apr 2015 10:02:17 +0900 Subject: [PATCH] scsi: ufs: fix ufshcd_set_dev_pwr_mode() when unloading module --- drivers/scsi/ufs/ufshcd.c | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 540e00d..91cbc04 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4695,23 +4695,19 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, struct scsi_sense_hdr sshdr; struct scsi_device *sdp; unsigned long flags; - int ret; + int ret = 0; + mutex_lock(hba-host-scan_mutex); spin_lock_irqsave(hba-host-host_lock, flags); sdp = hba-sdev_ufs_device; - if (sdp) { - ret = scsi_device_get(sdp); - if (!ret !scsi_device_online(sdp)) { - ret = -ENODEV; - scsi_device_put(sdp); - } - } else { + if (!sdp || !scsi_device_online(sdp)) ret = -ENODEV; - } spin_unlock_irqrestore(hba-host-host_lock, flags); - if (ret) + if (ret) { + mutex_unlock(hba-host-scan_mutex); return ret; + } /* * If scsi commands fail, the scsi mid-layer schedules scsi error- @@ -4748,7 +4744,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba, if (!ret) hba-curr_dev_pwr_mode = pwr_mode; out: - scsi_device_put(sdp); + mutex_unlock(hba-host-scan_mutex); hba-host-eh_noresume = 0; return ret; } -- 1.9.1
Re: [PATCH] ufs: Reinstate NULL pointer checks for regulators
2015-04-16 4:10 GMT+09:00 Bjorn Andersson bjorn.anders...@sonymobile.com: The cleanup during the introduction of regulator_set_load() was a too optimistic and re-opened an issue with vreg being dereferenced while being NULL. So reinstate the NULL checks and add back the BUG_ON() to follow the general coding convention of the implementation. Fixes: 7b16a07c3293 (ufs: Rename of regulator_set_optimum_mode) Reported-by: Akinobu Mita akinobu.m...@gmail.com Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com Tested-by: Akinobu Mita akinobu.m...@gmail.com This patch still isn't picked up by scsi tree nor regulator tree. James, Mark, Could you consider taking care of this patch? --- drivers/scsi/ufs/ufshcd.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 648a44675880..540e00df6de1 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4262,8 +4262,7 @@ static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg, { int ret; - if (!vreg) - return 0; + BUG_ON(!vreg); ret = regulator_set_load(vreg-reg, ua); if (ret 0) { @@ -4277,12 +4276,18 @@ static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg, static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba, struct ufs_vreg *vreg) { + if (!vreg) + return 0; + return ufshcd_config_vreg_load(hba-dev, vreg, UFS_VREG_LPM_LOAD_UA); } static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, struct ufs_vreg *vreg) { + if (!vreg) + return 0; + return ufshcd_config_vreg_load(hba-dev, vreg, vreg-max_uA); } -- 1.8.2.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] scatterlist: enable sg chaining for all architectures
2015-04-29 7:16 GMT+09:00 James Bottomley james.bottom...@hansenpartnership.com: On Tue, 2015-04-28 at 14:27 -0700, Andrew Morton wrote: On Sat, 25 Apr 2015 23:56:16 +0900 Akinobu Mita akinobu.m...@gmail.com wrote: Some architectures enable sg chaining option while others do not. The requirement to enable sg chaining is that pages must be aligned at a 32-bit boundary in order to overload the LSB of the pointer. Regardless of whether ARCH_HAS_SG_CHAIN is defined or not, the above requirement is always chacked by BUG_ON() in sg_assign_page. So all architectures can enable sg chaining. As you can see from the changes in drivers/target/target_core_rd.c, enabling SG chaining for all architectures allows us to allocate discontiguous scatterlist tables which can be traversed throughout by sg_next() without a special handling for some architectures. Thanks, I'll grab this. If anyone has concerns, speak now or hold both pieces! It breaks a host of architectures doesn't it? I can specifically speak for PARISC: The problem is the way our iommus are consuming scatterlists. They're assuming we can dereference the scatterlist as an array (like this code in ccio-dma.c): static int ccio_map_sg(struct device *dev, struct scatterlist *sglist, int nents, enum dma_data_direction direction) [...] for(i = 0; i nents; i++) prev_len += sglist[i].length; If you turn on sg chaining on our architecture, we'll run off the end of that array dereference and crash. This can all be fixed by making our architecture dma mapping code use iterators instead of array lists, but that needs more code than this patch provides. I assume there are similar issues on a lot of other architectures, so before you can contemplate a patch like this, surely all the architecture consumers have to be converted to iterator instead of array format? The first place to start would be a survey of who's still using the array format. Agreed. I could find similar issues in arch/m68k/kernel/dma.c. (git grep '[^a-z]sg++' shows that there are a lot of similar issues) Andrew, could you drop this patch from -mm for now? -- 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 v3 1/5] target: ensure se_cmd-t_prot_sg is allocated when required
2015-04-26 18:44 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: @@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd) static inline void transport_free_pages(struct se_cmd *cmd) { +if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { +transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); +cmd-t_prot_sg = NULL; +cmd-t_prot_nents = 0; +} + Hi Akinobu, Any reason why this changed it's location to the start of the function? Because when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is set, it will not reach the tail of the function. So when SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC is cleared and SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is set, se_cmd-t_prot_sg leaks. if (cmd-se_cmd_flags SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { /* * Release special case READ buffer payload required for @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd) transport_free_sgl(cmd-t_bidi_data_sg, cmd-t_bidi_data_nents); cmd-t_bidi_data_sg = NULL; cmd-t_bidi_data_nents = 0; - -transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); -cmd-t_prot_sg = NULL; -cmd-t_prot_nents = 0; } /** @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) int ret = 0; bool zero_flag = !(cmd-se_cmd_flags SCF_SCSI_DATA_CDB); +if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { +if (cmd-prot_op != TARGET_PROT_NORMAL) { This seems wrong, What will happen for transports that will actually to allocate protection SGLs? The allocation is unreachable since SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC is not set... Umm, actually this is reachable... But I still think the condition should be the other way around (saving a condition in some common cases). Do you mean you prefer below? if (cmd-prot_op != TARGET_PROT_NORMAL !(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { ... I'd say this needs to be: if (cmd-prot_op != TARGET_PROT_NORMAL !(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { -- 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 v3 5/5] target/file: enable WRITE SAME when protection info is enabled
2015-04-26 18:58 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/25/2015 5:33 PM, Akinobu Mita wrote: Now we can generate correct PI for WRITE SAME command, so it is unnecessary to disallow WRITE SAME when protection info is enabled. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * No change from v2 drivers/target/target_core_file.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 829817a..fe98f58 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -376,16 +376,12 @@ fd_execute_write_same(struct se_cmd *cmd) struct bio_vec *bvec; unsigned int len = 0, i; ssize_t ret; + sense_reason_t rc; if (!nolb) { target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } - if (cmd-prot_op) { - pr_err(WRITE_SAME: Protection information with FILEIO - backends not supported\n); - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } if (cmd-t_data_nents 1 || cmd-t_data_sg[0].length != cmd-se_dev-dev_attrib.block_size) { @@ -397,6 +393,10 @@ fd_execute_write_same(struct se_cmd *cmd) return TCM_INVALID_CDB_FIELD; } + rc = sbc_dif_verify(cmd, cmd-t_task_lba, nolb, 0, cmd-t_prot_sg, 0); + if (rc) + return rc; + bvec = kcalloc(nolb, sizeof(struct bio_vec), GFP_KERNEL); if (!bvec) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -418,6 +418,14 @@ fd_execute_write_same(struct se_cmd *cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } + if (cmd-prot_op) { + ret = fd_do_rw(cmd, fd_dev-fd_prot_file, se_dev-prot_length, + cmd-t_prot_sg, cmd-t_prot_nents, + cmd-prot_length, 1); + if (ret 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } This looks good, As you pointed out in the other mail, this change doesn't work with a real HW fabric because it doesn't generate multiple same protection fields for a single data block currently. So I'm considering dropping this from this patch series for now. iblock is needed too though. I think you just need a missing call to iblock_alloc_bip() and you're good to go (you can use scsi_debug with dif/dix to test it). I think it belongs in the same patch. Thanks for the information. I'll take a look. -- 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 v3 3/5] target: handle odd SG mapping for data transfer memory
2015-04-26 19:07 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/25/2015 5:33 PM, Akinobu Mita wrote: sbc_dif_generate() and sbc_dif_verify() currently assume that each SG element for data transfer memory doesn't straddle the block size boundary. However, when using SG_IO ioctl, we can choose the data transfer memory which doesn't satisfy that alignment requirement. In order to handle such cases correctly, this change inverts the outer loop to iterate data transfer memory and the inner loop to iterate protection information and enables to calculate CRC for a block which straddles multiple SG elements. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Tim Chen tim.c.c...@linux.intel.com Cc: Herbert Xu herb...@gondor.apana.org.au Cc: David S. Miller da...@davemloft.net Cc: linux-cry...@vger.kernel.org Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes from v2: - Handle odd SG mapping correctly instead of giving up drivers/target/target_core_sbc.c | 108 +-- 1 file changed, 69 insertions(+), 39 deletions(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index edba39f..33d2426 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1182,27 +1182,43 @@ sbc_dif_generate(struct se_cmd *cmd) { struct se_device *dev = cmd-se_dev; struct se_dif_v1_tuple *sdt; - struct scatterlist *dsg, *psg = cmd-t_prot_sg; + struct scatterlist *dsg = cmd-t_data_sg, *psg; sector_t sector = cmd-t_task_lba; void *daddr, *paddr; int i, j, offset = 0; + unsigned int block_size = dev-dev_attrib.block_size; - for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { - daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; + for_each_sg(cmd-t_prot_sg, psg, cmd-t_prot_nents, i) { paddr = kmap_atomic(sg_page(psg)) + psg-offset; + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; - for (j = 0; j dsg-length; j += dev-dev_attrib.block_size) { + for (j = 0; j psg-length; + j += sizeof(struct se_dif_v1_tuple)) { + __u16 crc = 0; + unsigned int avail; - if (offset = psg-length) { - kunmap_atomic(paddr); - psg = sg_next(psg); - paddr = kmap_atomic(sg_page(psg)) + psg-offset; - offset = 0; + if (offset = dsg-length) { + offset -= dsg-length; + kunmap_atomic(daddr); This unmap is inconsistent. You need to unmap (daddr - dsg-offset). This applies throughout the patch. Thanks for pointing out. I'll fix them all. -- 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 v3 4/5] target: Fix sbc_dif_generate() and sbc_dif_verify() for WRITE SAME
2015-04-26 18:53 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/25/2015 5:33 PM, Akinobu Mita wrote: For WRITE SAME, data transfer memory only contains a single block but protection information is required for all blocks that are written by the command. This makes sbc_dif_generate() and sbc_dif_verify() work for WRITE_SAME. This feels a bit like an overshoot... You only have 1 block, is it really a good idea to calculate the CRC over and over for write same? Wouldn't it be better to have a really simple sbc_dif_generate_same() that calculates the block CRC once and uses it for the entire payload (and watches for Type 1 to increment the ref-tag)? Sounds good. I'll take the idea. -- 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 v3 4/5] target: Fix sbc_dif_generate() and sbc_dif_verify() for WRITE SAME
For WRITE SAME, data transfer memory only contains a single block but protection information is required for all blocks that are written by the command. This makes sbc_dif_generate() and sbc_dif_verify() work for WRITE_SAME. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes from v2: - Handle odd SG mapping correctly instead of giving up drivers/target/target_core_sbc.c | 28 1 file changed, 28 insertions(+) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index 33d2426..10c7cb9 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1177,6 +1177,24 @@ err: } EXPORT_SYMBOL(sbc_execute_unmap); +static bool sbc_is_write_same(struct se_cmd *cmd) +{ + u16 service_action; + + switch (cmd-t_task_cdb[0]) { + case WRITE_SAME: + case WRITE_SAME_16: + return true; + case VARIABLE_LENGTH_CMD: + service_action = get_unaligned_be16(cmd-t_task_cdb[8]); + if (service_action == WRITE_SAME_32) + return true; + break; + } + + return false; +} + void sbc_dif_generate(struct se_cmd *cmd) { @@ -1187,6 +1205,7 @@ sbc_dif_generate(struct se_cmd *cmd) void *daddr, *paddr; int i, j, offset = 0; unsigned int block_size = dev-dev_attrib.block_size; + bool is_write_same = sbc_is_write_same(cmd); for_each_sg(cmd-t_prot_sg, psg, cmd-t_prot_nents, i) { paddr = kmap_atomic(sg_page(psg)) + psg-offset; @@ -1201,6 +1220,8 @@ sbc_dif_generate(struct se_cmd *cmd) offset -= dsg-length; kunmap_atomic(daddr); dsg = sg_next(dsg); + if (!dsg is_write_same) + dsg = cmd-t_data_sg; daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; } @@ -1211,6 +1232,8 @@ sbc_dif_generate(struct se_cmd *cmd) if (avail block_size) { kunmap_atomic(daddr); dsg = sg_next(dsg); + if (!dsg is_write_same) + dsg = cmd-t_data_sg; daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; offset = block_size - avail; crc = crc_t10dif_update(crc, daddr, offset); @@ -1336,6 +1359,7 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, sense_reason_t rc; int dsg_off = 0; unsigned int block_size = dev-dev_attrib.block_size; + bool is_write_same = sbc_is_write_same(cmd); for (; psg sector start + sectors; psg = sg_next(psg)) { paddr = kmap_atomic(sg_page(psg)) + psg-offset; @@ -1351,6 +1375,8 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, dsg_off -= dsg-length; kunmap_atomic(daddr); dsg = sg_next(dsg); + if (!dsg is_write_same) + dsg = cmd-t_data_sg; daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; } @@ -1372,6 +1398,8 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, if (avail block_size) { kunmap_atomic(daddr); dsg = sg_next(dsg); + if (!dsg is_write_same) + dsg = cmd-t_data_sg; daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; dsg_off = block_size - avail; crc = crc_t10dif_update(crc, daddr, dsg_off); -- 1.9.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 v3 3/5] target: handle odd SG mapping for data transfer memory
sbc_dif_generate() and sbc_dif_verify() currently assume that each SG element for data transfer memory doesn't straddle the block size boundary. However, when using SG_IO ioctl, we can choose the data transfer memory which doesn't satisfy that alignment requirement. In order to handle such cases correctly, this change inverts the outer loop to iterate data transfer memory and the inner loop to iterate protection information and enables to calculate CRC for a block which straddles multiple SG elements. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Tim Chen tim.c.c...@linux.intel.com Cc: Herbert Xu herb...@gondor.apana.org.au Cc: David S. Miller da...@davemloft.net Cc: linux-cry...@vger.kernel.org Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes from v2: - Handle odd SG mapping correctly instead of giving up drivers/target/target_core_sbc.c | 108 +-- 1 file changed, 69 insertions(+), 39 deletions(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index edba39f..33d2426 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1182,27 +1182,43 @@ sbc_dif_generate(struct se_cmd *cmd) { struct se_device *dev = cmd-se_dev; struct se_dif_v1_tuple *sdt; - struct scatterlist *dsg, *psg = cmd-t_prot_sg; + struct scatterlist *dsg = cmd-t_data_sg, *psg; sector_t sector = cmd-t_task_lba; void *daddr, *paddr; int i, j, offset = 0; + unsigned int block_size = dev-dev_attrib.block_size; - for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { - daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; + for_each_sg(cmd-t_prot_sg, psg, cmd-t_prot_nents, i) { paddr = kmap_atomic(sg_page(psg)) + psg-offset; + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; - for (j = 0; j dsg-length; j += dev-dev_attrib.block_size) { + for (j = 0; j psg-length; + j += sizeof(struct se_dif_v1_tuple)) { + __u16 crc = 0; + unsigned int avail; - if (offset = psg-length) { - kunmap_atomic(paddr); - psg = sg_next(psg); - paddr = kmap_atomic(sg_page(psg)) + psg-offset; - offset = 0; + if (offset = dsg-length) { + offset -= dsg-length; + kunmap_atomic(daddr); + dsg = sg_next(dsg); + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; } - sdt = paddr + offset; - sdt-guard_tag = cpu_to_be16(crc_t10dif(daddr + j, - dev-dev_attrib.block_size)); + sdt = paddr + j; + + avail = min(block_size, dsg-length - offset); + crc = crc_t10dif(daddr + offset, avail); + if (avail block_size) { + kunmap_atomic(daddr); + dsg = sg_next(dsg); + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; + offset = block_size - avail; + crc = crc_t10dif_update(crc, daddr, offset); + } else { + offset += block_size; + } + + sdt-guard_tag = cpu_to_be16(crc); if (cmd-prot_type == TARGET_DIF_TYPE1_PROT) sdt-ref_tag = cpu_to_be32(sector 0x); sdt-app_tag = 0; @@ -1215,26 +1231,23 @@ sbc_dif_generate(struct se_cmd *cmd) be32_to_cpu(sdt-ref_tag)); sector++; - offset += sizeof(struct se_dif_v1_tuple); } - kunmap_atomic(paddr); kunmap_atomic(daddr); + kunmap_atomic(paddr); } } static sense_reason_t sbc_dif_v1_verify(struct se_cmd *cmd, struct se_dif_v1_tuple *sdt, - const void *p, sector_t sector, unsigned int ei_lba) + __u16 crc, sector_t sector, unsigned int ei_lba) { - struct se_device *dev = cmd-se_dev; - int block_size = dev-dev_attrib.block_size; __be16 csum; if (!(cmd-prot_checks TARGET_DIF_CHECK_GUARD)) goto check_ref; - csum = cpu_to_be16(crc_t10dif(p, block_size
[PATCH v3 1/5] target: ensure se_cmd-t_prot_sg is allocated when required
Even if the device backend is initialized with protection info is enabled, some requests don't have the protection info attached for WRITE SAME command issued by block device helpers, WRITE command with WRPROTECT=0 by SG_IO ioctl, etc. So when TCM loopback fabric module is used, se_cmd-t_prot_sg is NULL for these requests and performing WRITE_INSERT of PI using software emulation by sbc_dif_generate() causes kernel crash. To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for se_cmd_flags, which is used to determine that se_cmd-t_prot_sg needs to be allocated or use pre-allocated protection information by scsi mid-layer. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * No change from v2 drivers/target/target_core_transport.c | 30 ++ include/target/target_core_base.h | 1 + 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7a9e7e2..fe52883 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess if (sgl_prot_count) { se_cmd-t_prot_sg = sgl_prot; se_cmd-t_prot_nents = sgl_prot_count; + se_cmd-se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC; } /* @@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd) static inline void transport_free_pages(struct se_cmd *cmd) { + if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); + cmd-t_prot_sg = NULL; + cmd-t_prot_nents = 0; + } + if (cmd-se_cmd_flags SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { /* * Release special case READ buffer payload required for @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd) transport_free_sgl(cmd-t_bidi_data_sg, cmd-t_bidi_data_nents); cmd-t_bidi_data_sg = NULL; cmd-t_bidi_data_nents = 0; - - transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); - cmd-t_prot_sg = NULL; - cmd-t_prot_nents = 0; } /** @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) int ret = 0; bool zero_flag = !(cmd-se_cmd_flags SCF_SCSI_DATA_CDB); + if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + if (cmd-prot_op != TARGET_PROT_NORMAL) { + ret = target_alloc_sgl(cmd-t_prot_sg, + cmd-t_prot_nents, + cmd-prot_length, true); + if (ret 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + + } + /* * Determine is the TCM fabric module has already allocated physical * memory, and is directly calling transport_generic_map_mem_to_cmd() @@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } - if (cmd-prot_op != TARGET_PROT_NORMAL) { - ret = target_alloc_sgl(cmd-t_prot_sg, - cmd-t_prot_nents, - cmd-prot_length, true); - if (ret 0) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } - ret = target_alloc_sgl(cmd-t_data_sg, cmd-t_data_nents, cmd-data_length, zero_flag); if (ret 0) diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 480e9f8..13efcdd 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -167,6 +167,7 @@ enum se_cmd_flags_table { SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x0002, SCF_COMPARE_AND_WRITE = 0x0008, SCF_COMPARE_AND_WRITE_POST = 0x0010, + SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x0020, }; /* struct se_dev_entry-lun_flags and struct se_lun-lun_access */ -- 1.9.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 v3 5/5] target/file: enable WRITE SAME when protection info is enabled
Now we can generate correct PI for WRITE SAME command, so it is unnecessary to disallow WRITE SAME when protection info is enabled. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * No change from v2 drivers/target/target_core_file.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 829817a..fe98f58 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -376,16 +376,12 @@ fd_execute_write_same(struct se_cmd *cmd) struct bio_vec *bvec; unsigned int len = 0, i; ssize_t ret; + sense_reason_t rc; if (!nolb) { target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } - if (cmd-prot_op) { - pr_err(WRITE_SAME: Protection information with FILEIO - backends not supported\n); - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } if (cmd-t_data_nents 1 || cmd-t_data_sg[0].length != cmd-se_dev-dev_attrib.block_size) { @@ -397,6 +393,10 @@ fd_execute_write_same(struct se_cmd *cmd) return TCM_INVALID_CDB_FIELD; } + rc = sbc_dif_verify(cmd, cmd-t_task_lba, nolb, 0, cmd-t_prot_sg, 0); + if (rc) + return rc; + bvec = kcalloc(nolb, sizeof(struct bio_vec), GFP_KERNEL); if (!bvec) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -418,6 +418,14 @@ fd_execute_write_same(struct se_cmd *cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } + if (cmd-prot_op) { + ret = fd_do_rw(cmd, fd_dev-fd_prot_file, se_dev-prot_length, + cmd-t_prot_sg, cmd-t_prot_nents, + cmd-prot_length, 1); + if (ret 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } -- 1.9.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] scatterlist: enable sg chaining for all architectures
Some architectures enable sg chaining option while others do not. The requirement to enable sg chaining is that pages must be aligned at a 32-bit boundary in order to overload the LSB of the pointer. Regardless of whether ARCH_HAS_SG_CHAIN is defined or not, the above requirement is always chacked by BUG_ON() in sg_assign_page. So all architectures can enable sg chaining. As you can see from the changes in drivers/target/target_core_rd.c, enabling SG chaining for all architectures allows us to allocate discontiguous scatterlist tables which can be traversed throughout by sg_next() without a special handling for some architectures. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Arnd Bergmann a...@arndb.de Cc: linux-a...@vger.kernel.org Cc: James E.J. Bottomley jbottom...@odin.com Cc: Christoph Hellwig h...@lst.de Cc: linux-scsi@vger.kernel.org Cc: Nicholas A. Bellinger n...@linux-iscsi.org Cc: target-de...@vger.kernel.org --- arch/arm/Kconfig| 6 -- arch/arm64/Kconfig | 1 - arch/ia64/Kconfig | 1 - arch/powerpc/Kconfig| 1 - arch/s390/Kconfig | 1 - arch/sparc/Kconfig | 1 - arch/x86/Kconfig| 1 - drivers/target/target_core_rd.c | 45 - include/linux/scatterlist.h | 4 include/scsi/scsi.h | 8 ++-- lib/Kconfig | 7 --- lib/scatterlist.c | 8 12 files changed, 2 insertions(+), 82 deletions(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 45df48b..4436000 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -89,16 +89,11 @@ config ARM Europe. There is an ARM Linux project with a web page at http://www.arm.linux.org.uk/. -config ARM_HAS_SG_CHAIN - select ARCH_HAS_SG_CHAIN - bool - config NEED_SG_DMA_LENGTH bool config ARM_DMA_USE_IOMMU bool - select ARM_HAS_SG_CHAIN select NEED_SG_DMA_LENGTH if ARM_DMA_USE_IOMMU @@ -318,7 +313,6 @@ config ARCH_MULTIPLATFORM bool Allow multiple platforms to be selected depends on MMU select ARCH_WANT_OPTIONAL_GPIOLIB - select ARM_HAS_SG_CHAIN select ARM_PATCH_PHYS_VIRT select AUTO_ZRELADDR select CLKSRC_OF diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 4269dba..582462a 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -5,7 +5,6 @@ config ARM64 select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_GCOV_PROFILE_ALL - select ARCH_HAS_SG_CHAIN select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST select ARCH_USE_CMPXCHG_LOCKREF select ARCH_SUPPORTS_ATOMIC_RMW diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig index 76d25b2..a9f896e 100644 --- a/arch/ia64/Kconfig +++ b/arch/ia64/Kconfig @@ -32,7 +32,6 @@ config IA64 select HAVE_MEMBLOCK select HAVE_MEMBLOCK_NODE_MAP select HAVE_VIRT_CPU_ACCOUNTING - select ARCH_HAS_SG_CHAIN select VIRT_TO_BUS select ARCH_DISCARD_MEMBLOCK select GENERIC_IRQ_PROBE diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 190cc48..6f3b6768 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -112,7 +112,6 @@ config PPC select HAVE_DMA_API_DEBUG select HAVE_OPROFILE select HAVE_DEBUG_KMEMLEAK - select ARCH_HAS_SG_CHAIN select GENERIC_ATOMIC64 if PPC32 select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE select HAVE_PERF_EVENTS diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index 8e58c61..2854e52 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -67,7 +67,6 @@ config S390 select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS select ARCH_HAS_ELF_RANDOMIZE select ARCH_HAS_GCOV_PROFILE_ALL - select ARCH_HAS_SG_CHAIN select ARCH_HAVE_NMI_SAFE_CMPXCHG select ARCH_INLINE_READ_LOCK select ARCH_INLINE_READ_LOCK_BH diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig index e49502a..d0512f5 100644 --- a/arch/sparc/Kconfig +++ b/arch/sparc/Kconfig @@ -42,7 +42,6 @@ config SPARC select MODULES_USE_ELF_RELA select ODD_RT_SIGACTION select OLD_SIGSUSPEND - select ARCH_HAS_SG_CHAIN config SPARC32 def_bool !64BIT diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 226d569..eeb52b8 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -101,7 +101,6 @@ config X86 select HAVE_BPF_JIT if X86_64 select HAVE_ARCH_TRANSPARENT_HUGEPAGE select HAVE_ARCH_HUGE_VMAP if X86_64 || (X86_32 X86_PAE) - select ARCH_HAS_SG_CHAIN select CLKEVT_I8253 select ARCH_HAVE_NMI_SAFE_CMPXCHG select GENERIC_IOMAP diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index fa20d67..ce6a0ac 100644 --- a/drivers/target
[PATCH v2 2/4] target: Abandon odd SG mapping for data transfer memory
sbc_dif_generate() and sbc_dif_verify() assume that each SG element for data transfer memory doesn't straddle the block size boundary. However, when using SG_IO ioctl, we can choose the data transfer memory which doesn't satisfy that alignment requirement. This change detects such cases and makes those functions return failure to stop continuing the operation. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes from v1: - Reduce code duplication a bit in target_read_prot_action() drivers/target/target_core_sbc.c | 10 +- drivers/target/target_core_transport.c | 23 +++ include/target/target_core_backend.h | 2 +- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index edba39f..982836b 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1177,7 +1177,7 @@ err: } EXPORT_SYMBOL(sbc_execute_unmap); -void +sense_reason_t sbc_dif_generate(struct se_cmd *cmd) { struct se_device *dev = cmd-se_dev; @@ -1188,6 +1188,9 @@ sbc_dif_generate(struct se_cmd *cmd) int i, j, offset = 0; for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { + if (dsg-length % dev-dev_attrib.block_size) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; paddr = kmap_atomic(sg_page(psg)) + psg-offset; @@ -1221,6 +1224,8 @@ sbc_dif_generate(struct se_cmd *cmd) kunmap_atomic(paddr); kunmap_atomic(daddr); } + + return 0; } static sense_reason_t @@ -1323,6 +1328,9 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, sense_reason_t rc; for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { + if (dsg-length % dev-dev_attrib.block_size) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; paddr = kmap_atomic(sg_page(psg)) + psg-offset; diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index fe52883..fd89f57 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1760,7 +1760,7 @@ static int target_write_prot_action(struct se_cmd *cmd) switch (cmd-prot_op) { case TARGET_PROT_DOUT_INSERT: if (!(cmd-se_sess-sup_prot_ops TARGET_PROT_DOUT_INSERT)) - sbc_dif_generate(cmd); + cmd-pi_err = sbc_dif_generate(cmd); break; case TARGET_PROT_DOUT_STRIP: if (cmd-se_sess-sup_prot_ops TARGET_PROT_DOUT_STRIP) @@ -1769,18 +1769,19 @@ static int target_write_prot_action(struct se_cmd *cmd) sectors = cmd-data_length ilog2(cmd-se_dev-dev_attrib.block_size); cmd-pi_err = sbc_dif_verify(cmd, cmd-t_task_lba, sectors, 0, cmd-t_prot_sg, 0); - if (unlikely(cmd-pi_err)) { - spin_lock_irq(cmd-t_state_lock); - cmd-transport_state = ~CMD_T_BUSY|CMD_T_SENT; - spin_unlock_irq(cmd-t_state_lock); - transport_generic_request_failure(cmd, cmd-pi_err); - return -1; - } break; default: break; } + if (unlikely(cmd-pi_err)) { + spin_lock_irq(cmd-t_state_lock); + cmd-transport_state = ~CMD_T_BUSY|CMD_T_SENT; + spin_unlock_irq(cmd-t_state_lock); + transport_generic_request_failure(cmd, cmd-pi_err); + return -1; + } + return 0; } @@ -2002,21 +2003,19 @@ static bool target_read_prot_action(struct se_cmd *cmd) cmd-pi_err = sbc_dif_verify(cmd, cmd-t_task_lba, sectors, 0, cmd-t_prot_sg, 0); - if (cmd-pi_err) - return true; } break; case TARGET_PROT_DIN_INSERT: if (cmd-se_sess-sup_prot_ops TARGET_PROT_DIN_INSERT) break; - sbc_dif_generate(cmd); + cmd-pi_err = sbc_dif_generate(cmd); break; default: break; } - return false; + return cmd-pi_err ? true : false; } static void
[PATCH v2 4/4] target/file: enable WRITE SAME when protection info is enabled
Now we can generate correct PI for WRITE SAME command, so it is unnecessary to disallow WRITE SAME when protection info is enabled. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes from v1: - Fix inverted rw argument for fd_do_rw() - Perform DIF verify before write for WRITE_SAME drivers/target/target_core_file.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 829817a..fe98f58 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -376,16 +376,12 @@ fd_execute_write_same(struct se_cmd *cmd) struct bio_vec *bvec; unsigned int len = 0, i; ssize_t ret; + sense_reason_t rc; if (!nolb) { target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } - if (cmd-prot_op) { - pr_err(WRITE_SAME: Protection information with FILEIO - backends not supported\n); - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } if (cmd-t_data_nents 1 || cmd-t_data_sg[0].length != cmd-se_dev-dev_attrib.block_size) { @@ -397,6 +393,10 @@ fd_execute_write_same(struct se_cmd *cmd) return TCM_INVALID_CDB_FIELD; } + rc = sbc_dif_verify(cmd, cmd-t_task_lba, nolb, 0, cmd-t_prot_sg, 0); + if (rc) + return rc; + bvec = kcalloc(nolb, sizeof(struct bio_vec), GFP_KERNEL); if (!bvec) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -418,6 +418,14 @@ fd_execute_write_same(struct se_cmd *cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } + if (cmd-prot_op) { + ret = fd_do_rw(cmd, fd_dev-fd_prot_file, se_dev-prot_length, + cmd-t_prot_sg, cmd-t_prot_nents, + cmd-prot_length, 1); + if (ret 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } -- 1.9.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 v2 3/4] target: Fix sbc_dif_generate() and sbc_dif_verify() for WRITE SAME
For WRITE SAME, data transfer memory only contains a single block but protection information is required for all blocks that are written by the command. This makes sbc_dif_generate() and sbc_dif_verify() work for WRITE_SAME. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes from v1: - Fix sbc_dif_verify() for WRITE_SAME command drivers/target/target_core_sbc.c | 39 ++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index 982836b..5577829 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1177,6 +1177,24 @@ err: } EXPORT_SYMBOL(sbc_execute_unmap); +static bool sbc_is_write_same(struct se_cmd *cmd) +{ + u16 service_action; + + switch (cmd-t_task_cdb[0]) { + case WRITE_SAME: + case WRITE_SAME_16: + return true; + case VARIABLE_LENGTH_CMD: + service_action = get_unaligned_be16(cmd-t_task_cdb[8]); + if (service_action == WRITE_SAME_32) + return true; + break; + } + + return false; +} + sense_reason_t sbc_dif_generate(struct se_cmd *cmd) { @@ -1185,8 +1203,14 @@ sbc_dif_generate(struct se_cmd *cmd) struct scatterlist *dsg, *psg = cmd-t_prot_sg; sector_t sector = cmd-t_task_lba; void *daddr, *paddr; - int i, j, offset = 0; + int i, j, offset = 0, left = cmd-prot_length; + bool is_write_same = sbc_is_write_same(cmd); + if (is_write_same (cmd-t_data_nents != 1 || + cmd-t_data_sg-length != dev-dev_attrib.block_size)) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + +repeat: for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { if (dsg-length % dev-dev_attrib.block_size) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -1219,11 +1243,14 @@ sbc_dif_generate(struct se_cmd *cmd) sector++; offset += sizeof(struct se_dif_v1_tuple); + left -= sizeof(struct se_dif_v1_tuple); } kunmap_atomic(paddr); kunmap_atomic(daddr); } + if (is_write_same left) + goto repeat; return 0; } @@ -1326,7 +1353,14 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, void *daddr, *paddr; int i, j; sense_reason_t rc; + int left = cmd-prot_length; + bool is_write_same = sbc_is_write_same(cmd); + + if (is_write_same (cmd-t_data_nents != 1 || + cmd-t_data_sg-length != dev-dev_attrib.block_size)) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; +repeat: for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { if (dsg-length % dev-dev_attrib.block_size) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -1368,11 +1402,14 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, sector++; ei_lba++; psg_off += sizeof(struct se_dif_v1_tuple); + left -= sizeof(struct se_dif_v1_tuple); } kunmap_atomic(paddr - psg-offset); kunmap_atomic(daddr - dsg-offset); } + if (is_write_same left) + goto repeat; return 0; } -- 1.9.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 v2 1/4] target: ensure se_cmd-t_prot_sg is allocated when required
Even if the device backend is initialized with protection info is enabled, some requests don't have the protection info attached for WRITE SAME command issued by block device helpers, WRITE command with WRPROTECT=0 by SG_IO ioctl, etc. So when TCM loopback fabric module is used, se_cmd-t_prot_sg is NULL for these requests and performing WRITE_INSERT of PI using software emulation by sbc_dif_generate() causes kernel crash. To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for se_cmd_flags, which is used to determine that se_cmd-t_prot_sg needs to be allocated or use pre-allocated protection information by scsi mid-layer. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * No change from v1 drivers/target/target_core_transport.c | 30 ++ include/target/target_core_base.h | 1 + 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7a9e7e2..fe52883 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess if (sgl_prot_count) { se_cmd-t_prot_sg = sgl_prot; se_cmd-t_prot_nents = sgl_prot_count; + se_cmd-se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC; } /* @@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd) static inline void transport_free_pages(struct se_cmd *cmd) { + if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); + cmd-t_prot_sg = NULL; + cmd-t_prot_nents = 0; + } + if (cmd-se_cmd_flags SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { /* * Release special case READ buffer payload required for @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd) transport_free_sgl(cmd-t_bidi_data_sg, cmd-t_bidi_data_nents); cmd-t_bidi_data_sg = NULL; cmd-t_bidi_data_nents = 0; - - transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); - cmd-t_prot_sg = NULL; - cmd-t_prot_nents = 0; } /** @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) int ret = 0; bool zero_flag = !(cmd-se_cmd_flags SCF_SCSI_DATA_CDB); + if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + if (cmd-prot_op != TARGET_PROT_NORMAL) { + ret = target_alloc_sgl(cmd-t_prot_sg, + cmd-t_prot_nents, + cmd-prot_length, true); + if (ret 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + + } + /* * Determine is the TCM fabric module has already allocated physical * memory, and is directly calling transport_generic_map_mem_to_cmd() @@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } - if (cmd-prot_op != TARGET_PROT_NORMAL) { - ret = target_alloc_sgl(cmd-t_prot_sg, - cmd-t_prot_nents, - cmd-prot_length, true); - if (ret 0) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } - ret = target_alloc_sgl(cmd-t_data_sg, cmd-t_data_nents, cmd-data_length, zero_flag); if (ret 0) diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 480e9f8..13efcdd 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -167,6 +167,7 @@ enum se_cmd_flags_table { SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x0002, SCF_COMPARE_AND_WRITE = 0x0008, SCF_COMPARE_AND_WRITE_POST = 0x0010, + SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x0020, }; /* struct se_dev_entry-lun_flags and struct se_lun-lun_access */ -- 1.9.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 v2 0/4] target: Fix several problems related to T10-PI support
This patchset aims to fix several problems related to T10-PI support. These patches can be applied on top of Sagi's [v1] Simlify dif_verify routines and fixup fileio protection information code patchset. * Changes from v1: - Reduce code duplication a bit in target_read_prot_action() - Fix sbc_dif_verify() for WRITE_SAME command - Fix inverted rw argument for fd_do_rw() - Perform DIF verify before write for WRITE_SAME Akinobu Mita (4): target: ensure se_cmd-t_prot_sg is allocated when required target: Abandon odd SG mapping for data transfer memory target: Fix sbc_dif_generate() and sbc_dif_verify() for WRITE SAME target/file: enable WRITE SAME when protection info is enabled drivers/target/target_core_file.c | 18 drivers/target/target_core_sbc.c | 49 +-- drivers/target/target_core_transport.c | 53 +++--- include/target/target_core_backend.h | 2 +- include/target/target_core_base.h | 1 + 5 files changed, 91 insertions(+), 32 deletions(-) Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org -- 1.9.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 4/4] target/file: enable WRITE SAME when protection info is enabled
2015-04-21 8:36 GMT+09:00 Akinobu Mita akinobu.m...@gmail.com: Now we can generate correct PI for WRITE SAME command, so it is unnecessary to disallow WRITE SAME when protection info is enabled. I noticed that this patch has multiple problems. @@ -381,11 +381,6 @@ fd_execute_write_same(struct se_cmd *cmd) target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } - if (cmd-prot_op) { - pr_err(WRITE_SAME: Protection information with FILEIO - backends not supported\n); - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } if (cmd-t_data_nents 1 || cmd-t_data_sg[0].length != cmd-se_dev-dev_attrib.block_size) { @@ -401,6 +396,14 @@ fd_execute_write_same(struct se_cmd *cmd) if (!bvec) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + if (cmd-prot_op) { + ret = fd_do_rw(cmd, fd_dev-fd_prot_file, se_dev-prot_length, + cmd-t_prot_sg, cmd-t_prot_nents, + cmd-prot_length, 0); The last argument should be '1' as this is write. and we need to perform DIF verify before writing. -- 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/4] target: ensure se_cmd-t_prot_sg is allocated when required
Even if the device backend is initialized with protection info is enabled, some requests don't have the protection info attached for WRITE SAME command issued by block device helpers, WRITE command with WRPROTECT=0 by SG_IO ioctl, etc. So when TCM loopback fabric module is used, se_cmd-t_prot_sg is NULL for these requests and performing WRITE_INSERT of PI using software emulation by sbc_dif_generate() causes kernel crash. To fix this, introduce SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC for se_cmd_flags, which is used to determine that se_cmd-t_prot_sg needs to be allocated or use pre-allocated protection information by scsi mid-layer. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_transport.c | 30 ++ include/target/target_core_base.h | 1 + 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 7a9e7e2..fe52883 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1450,6 +1450,7 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess if (sgl_prot_count) { se_cmd-t_prot_sg = sgl_prot; se_cmd-t_prot_nents = sgl_prot_count; + se_cmd-se_cmd_flags |= SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC; } /* @@ -2181,6 +2182,12 @@ static inline void transport_reset_sgl_orig(struct se_cmd *cmd) static inline void transport_free_pages(struct se_cmd *cmd) { + if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); + cmd-t_prot_sg = NULL; + cmd-t_prot_nents = 0; + } + if (cmd-se_cmd_flags SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC) { /* * Release special case READ buffer payload required for @@ -2204,10 +2211,6 @@ static inline void transport_free_pages(struct se_cmd *cmd) transport_free_sgl(cmd-t_bidi_data_sg, cmd-t_bidi_data_nents); cmd-t_bidi_data_sg = NULL; cmd-t_bidi_data_nents = 0; - - transport_free_sgl(cmd-t_prot_sg, cmd-t_prot_nents); - cmd-t_prot_sg = NULL; - cmd-t_prot_nents = 0; } /** @@ -2346,6 +2349,17 @@ transport_generic_new_cmd(struct se_cmd *cmd) int ret = 0; bool zero_flag = !(cmd-se_cmd_flags SCF_SCSI_DATA_CDB); + if (!(cmd-se_cmd_flags SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) { + if (cmd-prot_op != TARGET_PROT_NORMAL) { + ret = target_alloc_sgl(cmd-t_prot_sg, + cmd-t_prot_nents, + cmd-prot_length, true); + if (ret 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + + } + /* * Determine is the TCM fabric module has already allocated physical * memory, and is directly calling transport_generic_map_mem_to_cmd() @@ -2371,14 +2385,6 @@ transport_generic_new_cmd(struct se_cmd *cmd) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } - if (cmd-prot_op != TARGET_PROT_NORMAL) { - ret = target_alloc_sgl(cmd-t_prot_sg, - cmd-t_prot_nents, - cmd-prot_length, true); - if (ret 0) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } - ret = target_alloc_sgl(cmd-t_data_sg, cmd-t_data_nents, cmd-data_length, zero_flag); if (ret 0) diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 480e9f8..13efcdd 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -167,6 +167,7 @@ enum se_cmd_flags_table { SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x0002, SCF_COMPARE_AND_WRITE = 0x0008, SCF_COMPARE_AND_WRITE_POST = 0x0010, + SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x0020, }; /* struct se_dev_entry-lun_flags and struct se_lun-lun_access */ -- 1.9.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 0/4] target: Fix several problems related to T10-PI support
This patchset aims to fix several problems related to T10-PI support. These patches can be applied on top of Sagi's [v1] Simlify dif_verify routines and fixup fileio protection information code patchset. Akinobu Mita (4): target: ensure se_cmd-t_prot_sg is allocated when required target: Abandon odd SG mapping for data transfer memory target: Fix sbc_dif_generate() for WRITE SAME target/file: enable WRITE SAME when protection info is enabled drivers/target/target_core_file.c | 13 + drivers/target/target_core_sbc.c | 40 -- drivers/target/target_core_transport.c | 51 -- include/target/target_core_backend.h | 2 +- include/target/target_core_base.h | 1 + 5 files changed, 78 insertions(+), 29 deletions(-) Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org -- 1.9.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 2/4] target: Abandon odd SG mapping for data transfer memory
sbc_dif_generate() and sbc_dif_verify() assume that each SG element for data transfer memory doesn't straddle the block size boundary. However, when using SG_IO ioctl, we can choose the data transfer memory which doesn't satisfy that alignment requirement. This change detects such cases and makes those functions return failure to stop continuing the operation. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_sbc.c | 10 +- drivers/target/target_core_transport.c | 21 - include/target/target_core_backend.h | 2 +- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index edba39f..982836b 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1177,7 +1177,7 @@ err: } EXPORT_SYMBOL(sbc_execute_unmap); -void +sense_reason_t sbc_dif_generate(struct se_cmd *cmd) { struct se_device *dev = cmd-se_dev; @@ -1188,6 +1188,9 @@ sbc_dif_generate(struct se_cmd *cmd) int i, j, offset = 0; for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { + if (dsg-length % dev-dev_attrib.block_size) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; paddr = kmap_atomic(sg_page(psg)) + psg-offset; @@ -1221,6 +1224,8 @@ sbc_dif_generate(struct se_cmd *cmd) kunmap_atomic(paddr); kunmap_atomic(daddr); } + + return 0; } static sense_reason_t @@ -1323,6 +1328,9 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, sense_reason_t rc; for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { + if (dsg-length % dev-dev_attrib.block_size) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; paddr = kmap_atomic(sg_page(psg)) + psg-offset; diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index fe52883..d96852e 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1760,7 +1760,7 @@ static int target_write_prot_action(struct se_cmd *cmd) switch (cmd-prot_op) { case TARGET_PROT_DOUT_INSERT: if (!(cmd-se_sess-sup_prot_ops TARGET_PROT_DOUT_INSERT)) - sbc_dif_generate(cmd); + cmd-pi_err = sbc_dif_generate(cmd); break; case TARGET_PROT_DOUT_STRIP: if (cmd-se_sess-sup_prot_ops TARGET_PROT_DOUT_STRIP) @@ -1769,18 +1769,19 @@ static int target_write_prot_action(struct se_cmd *cmd) sectors = cmd-data_length ilog2(cmd-se_dev-dev_attrib.block_size); cmd-pi_err = sbc_dif_verify(cmd, cmd-t_task_lba, sectors, 0, cmd-t_prot_sg, 0); - if (unlikely(cmd-pi_err)) { - spin_lock_irq(cmd-t_state_lock); - cmd-transport_state = ~CMD_T_BUSY|CMD_T_SENT; - spin_unlock_irq(cmd-t_state_lock); - transport_generic_request_failure(cmd, cmd-pi_err); - return -1; - } break; default: break; } + if (unlikely(cmd-pi_err)) { + spin_lock_irq(cmd-t_state_lock); + cmd-transport_state = ~CMD_T_BUSY|CMD_T_SENT; + spin_unlock_irq(cmd-t_state_lock); + transport_generic_request_failure(cmd, cmd-pi_err); + return -1; + } + return 0; } @@ -2010,7 +2011,9 @@ static bool target_read_prot_action(struct se_cmd *cmd) if (cmd-se_sess-sup_prot_ops TARGET_PROT_DIN_INSERT) break; - sbc_dif_generate(cmd); + cmd-pi_err = sbc_dif_generate(cmd); + if (cmd-pi_err) + return true; break; default: break; diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h index ab8ed4c..41dfde0 100644 --- a/include/target/target_core_backend.h +++ b/include/target/target_core_backend.h @@ -85,7 +85,7 @@ sense_reason_t sbc_execute_unmap(struct se_cmd *cmd, sense_reason_t (*do_unmap_fn)(struct se_cmd *cmd, void *priv, sector_t lba, sector_t nolb), void *priv); -void sbc_dif_generate(struct se_cmd
[PATCH 4/4] target/file: enable WRITE SAME when protection info is enabled
Now we can generate correct PI for WRITE SAME command, so it is unnecessary to disallow WRITE SAME when protection info is enabled. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_file.c | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 829817a..200d9ec 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -381,11 +381,6 @@ fd_execute_write_same(struct se_cmd *cmd) target_complete_cmd(cmd, SAM_STAT_GOOD); return 0; } - if (cmd-prot_op) { - pr_err(WRITE_SAME: Protection information with FILEIO - backends not supported\n); - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - } if (cmd-t_data_nents 1 || cmd-t_data_sg[0].length != cmd-se_dev-dev_attrib.block_size) { @@ -401,6 +396,14 @@ fd_execute_write_same(struct se_cmd *cmd) if (!bvec) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + if (cmd-prot_op) { + ret = fd_do_rw(cmd, fd_dev-fd_prot_file, se_dev-prot_length, + cmd-t_prot_sg, cmd-t_prot_nents, + cmd-prot_length, 0); + if (ret 0) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + } + for (i = 0; i nolb; i++) { bvec[i].bv_page = sg_page(cmd-t_data_sg[0]); bvec[i].bv_len = cmd-t_data_sg[0].length; -- 1.9.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 3/4] target: Fix sbc_dif_generate() for WRITE SAME
For WRITE SAME, data transfer memory only contains a single block but protection information is required for all blocks that are written by the command. This makes sbc_dif_generate() work for WRITE_SAME. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_sbc.c | 30 +- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index 982836b..9815c1b 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1177,6 +1177,24 @@ err: } EXPORT_SYMBOL(sbc_execute_unmap); +static bool sbc_is_write_same(struct se_cmd *cmd) +{ + u16 service_action; + + switch (cmd-t_task_cdb[0]) { + case WRITE_SAME: + case WRITE_SAME_16: + return true; + case VARIABLE_LENGTH_CMD: + service_action = get_unaligned_be16(cmd-t_task_cdb[8]); + if (service_action == WRITE_SAME_32) + return true; + break; + } + + return false; +} + sense_reason_t sbc_dif_generate(struct se_cmd *cmd) { @@ -1185,8 +1203,15 @@ sbc_dif_generate(struct se_cmd *cmd) struct scatterlist *dsg, *psg = cmd-t_prot_sg; sector_t sector = cmd-t_task_lba; void *daddr, *paddr; - int i, j, offset = 0; + int i, j, offset = 0, left = cmd-prot_length; + bool is_write_same = sbc_is_write_same(cmd); + + if (is_write_same + (cmd-t_data_sg-length != dev-dev_attrib.block_size || +cmd-t_data_nents != 1)) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; +repeat: for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { if (dsg-length % dev-dev_attrib.block_size) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; @@ -1219,11 +1244,14 @@ sbc_dif_generate(struct se_cmd *cmd) sector++; offset += sizeof(struct se_dif_v1_tuple); + left -= sizeof(struct se_dif_v1_tuple); } kunmap_atomic(paddr); kunmap_atomic(daddr); } + if (is_write_same left) + goto repeat; return 0; } -- 1.9.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: [RFC] Simlify dif_verify routines and fixup fileio protection information code.
2015-04-16 17:52 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/15/2015 7:10 PM, Martin K. Petersen wrote: Sagi == Sagi Grimberg sa...@dev.mellanox.co.il writes: By the commit 436f4a0a (loopback: Add fabric_prot_type attribute support), When WRITE_SAME command with WRPROTECT=0 is executed, sbc_dif_generate() is called but cmd-t_prot_sg is NULL as block layer didn't allocate it for WRITE_SAME. Sagi Actually this is a bug. Why didn't the initiator allocate Sagi integrity meta-data for WRITE_SAME? Looking at the code it looks Sagi like it should. We don't issue WRITE SAME with PI so there is no prot SGL. Is there a specific reason why we don't? It is not only for the WRITE SAME requests from block device but also for READ/WRITE with PROTECT=0 requests by SG_IO. So isn't is appropreate to allocate prot SGL in target_write_prot_action() (and mark se_cmd-se_cmd_flags to release it at deallocation time)? -- 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] Simlify dif_verify routines and fixup fileio protection information code.
2015-04-15 19:07 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/15/2015 2:52 AM, Akinobu Mita wrote: Looks good... I'll test with these patches and check if the problems I met disappear. Thanks Akinobu, Waiting to hear your verdict before sending a formal patchset. I hit a original bug in sbc_dif_verify() which is not introduced by your patch set, though. What is this original bug? I meant to say about the problem I fixed by fix-sbc_dif_verify.patch. Please consider to include attached patch. I'll include it. thanks. I'm still seeing another problem and trying to find out a root cause, but it seems like it's caused by other change in -next. care to elaborate? When mounting ext4 filesystem at the first time after mkfs, a lot of WRITE_SAME commands are issued for lazy initialization or something. By the commit 436f4a0a (loopback: Add fabric_prot_type attribute support), When WRITE_SAME command with WRPROTECT=0 is executed, sbc_dif_generate() is called but cmd-t_prot_sg is NULL as block layer didn't allocate it for WRITE_SAME. I could work around with the attached patch, as the WRITE_SAME command will fail after all when protection info is enabled with FILEIO, we only need to avoid null pointer dereference. But I need to ask Nic about the right way to fix. For this patch set, please feel free to add: Tested-by: Akinobu Mita akinobu.m...@gmail.com diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index 3d88c00..d8d6267 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1183,6 +1183,9 @@ sbc_dif_generate(struct se_cmd *cmd) void *daddr, *paddr; int i, j, offset = 0; + if (!psg) + return; + for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; paddr = kmap_atomic(sg_page(psg)) + psg-offset;
Re: [PATCH 2/5] ufs: Rename of regulator_set_optimum_mode
2015-02-12 12:35 GMT+09:00 Bjorn Andersson bjorn.anders...@sonymobile.com: The function regulator_set_optimum_mode() is changing name to regulator_set_load(), so update the code accordingly. Also cleaned up ufshcd_config_vreg_load() while touching the code. Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com --- drivers/scsi/ufs/ufshcd.c | 27 +++ 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 2e4614b..4b73b94 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4225,22 +4225,15 @@ static struct scsi_host_template ufshcd_driver_template = { static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg, int ua) { - int ret = 0; - struct regulator *reg = vreg-reg; - const char *name = vreg-name; + int ret; - BUG_ON(!vreg); + if (!vreg) + return 0; - ret = regulator_set_optimum_mode(reg, ua); - if (ret = 0) { - /* -* regulator_set_optimum_mode() returns new regulator -* mode upon success. -*/ - ret = 0; - } else { - dev_err(dev, %s: %s set optimum mode(ua=%d) failed, err=%d\n, - __func__, name, ua, ret); + ret = regulator_set_load(vreg-reg, ua); + if (ret 0) { + dev_err(dev, %s: %s set load (ua=%d) failed, err=%d\n, + __func__, vreg-name, ua, ret); } return ret; @@ -4249,18 +4242,12 @@ static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg, static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba, struct ufs_vreg *vreg) { - if (!vreg) - return 0; - return ufshcd_config_vreg_load(hba-dev, vreg, UFS_VREG_LPM_LOAD_UA); } static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, struct ufs_vreg *vreg) { - if (!vreg) - return 0; - return ufshcd_config_vreg_load(hba-dev, vreg, vreg-max_uA); } I tried this patch and it caused kernel null pointer dereference with 'verg-max_uA' when vreg == NULL. So you can't simply move !vreg check here into ufshcd_config_vreg_load(). -- 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] Simlify dif_verify routines and fixup fileio protection information code.
2015-04-14 2:19 GMT+09:00 Sagi Grimberg sa...@mellanox.com: Hey All, This set follows the patchset from Akinobu Mita that addresses DIF bounce buffer sgl construction. Instead of trying to fix these bugs, this removes it altogether and work with cmd-t_prot_sg directly. The first patch is a simplification of the DIF verify varius routines leaving a single generic sbc_dif_verify that handles the protection information sgl we are working on. The second patch uses this simplification to remove the local prot_fd bounce buffer altogether. This passed minimal IO testing. Looks good... I'll test with these patches and check if the problems I met disappear. -- 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] Simlify dif_verify routines and fixup fileio protection information code.
2015-04-15 2:20 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/14/2015 3:17 PM, Akinobu Mita wrote: 2015-04-14 2:19 GMT+09:00 Sagi Grimberg sa...@mellanox.com: Hey All, This set follows the patchset from Akinobu Mita that addresses DIF bounce buffer sgl construction. Instead of trying to fix these bugs, this removes it altogether and work with cmd-t_prot_sg directly. The first patch is a simplification of the DIF verify varius routines leaving a single generic sbc_dif_verify that handles the protection information sgl we are working on. The second patch uses this simplification to remove the local prot_fd bounce buffer altogether. This passed minimal IO testing. Looks good... I'll test with these patches and check if the problems I met disappear. Thanks Akinobu, Waiting to hear your verdict before sending a formal patchset. I hit a original bug in sbc_dif_verify() which is not introduced by your patch set, though. Please consider to include attached patch. I'm still seeing another problem and trying to find out a root cause, but it seems like it's caused by other change in -next. diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index 3d88c00..65a0b5f 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -1311,7 +1311,7 @@ sbc_dif_verify(struct se_cmd *cmd, sector_t start, unsigned int sectors, for_each_sg(cmd-t_data_sg, dsg, cmd-t_data_nents, i) { daddr = kmap_atomic(sg_page(dsg)) + dsg-offset; - paddr = kmap_atomic(sg_page(psg)) + sg-offset; + paddr = kmap_atomic(sg_page(psg)) + psg-offset; for (j = 0; j dsg-length; j += dev-dev_attrib.block_size) {
Re: [PATCH -next v3] target/rd: Don't pass imcomplete scatterlist entries to sbc_dif_verify_*
2015-04-13 13:59 GMT+09:00 Nicholas A. Bellinger n...@linux-iscsi.org: On Sat, 2015-04-11 at 13:17 +0900, Akinobu Mita wrote: The scatterlist for protection information which is passed to sbc_dif_verify_read() or sbc_dif_verify_write() requires that neighboring scatterlist entries are contiguous or chained so that they can be iterated by sg_next(). However, the protection information for RD-MCP backends could be located in the multiple scatterlist arrays when the ramdisk space is too large. So if the read/write request straddles this boundary, sbc_dif_verify_read() or sbc_dif_verify_write() can't iterate all scatterlist entries. This problem can be fixed by chaining protection information scatterlist at creation time. For the architectures which don't support sg chaining (i.e. !CONFIG_ARCH_HAS_SG_CHAIN), fix it by allocating temporary scatterlist if needed. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@dev.mellanox.co.il Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * v3 - Fix it by chaining protection information scatterlist at creation time if CONFIG_ARCH_HAS_SG_CHAIN is defined. drivers/target/target_core_rd.c | 67 +++-- 1 file changed, 64 insertions(+), 3 deletions(-) Applied to target-pending/for-next. Thanks for updating to use CONFIG_ARCH_HAS_SG_CHAIN for the common case. :) BTW, does anyone know why some architectures disable CONFIG_ARCH_HAS_SG_CHAIN? As far as I can see, there is no additional requirement to enable it for those that currently disable it. -- 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/3] target/file: Fix BUG() when CONFIG_DEBUG_SG=y and DIF protection enabled
When CONFIG_DEBUG_SG=y and DIF protection support enabled, kernel BUG()s are triggered due to the following two issues: 1) prot_sg is not initialized by sg_init_table(). When CONFIG_DEBUG_SG=y, scatterlist helpers check sg entry has a correct magic value. 2) vmalloc'ed buffer is passed to sg_set_buf(). sg_set_buf() uses virt_to_page() to convert virtual address to struct page, but it doesn't work with vmalloc address. vmalloc_to_page() should be used instead. As prot_buf isn't usually too large, so fix it by allocating prot_buf by kmalloc instead of vmalloc. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@mellanox.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_file.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c index 44620fb..8ca1883 100644 --- a/drivers/target/target_core_file.c +++ b/drivers/target/target_core_file.c @@ -274,7 +274,7 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot, se_dev-prot_length; if (!is_write) { - fd_prot-prot_buf = vzalloc(prot_size); + fd_prot-prot_buf = kzalloc(prot_size, GFP_KERNEL); if (!fd_prot-prot_buf) { pr_err(Unable to allocate fd_prot-prot_buf\n); return -ENOMEM; @@ -286,9 +286,10 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot, fd_prot-prot_sg_nents, GFP_KERNEL); if (!fd_prot-prot_sg) { pr_err(Unable to allocate fd_prot-prot_sg\n); - vfree(fd_prot-prot_buf); + kfree(fd_prot-prot_buf); return -ENOMEM; } + sg_init_table(fd_prot-prot_sg, fd_prot-prot_sg_nents); size = prot_size; for_each_sg(fd_prot-prot_sg, sg, fd_prot-prot_sg_nents, i) { @@ -318,7 +319,7 @@ static int fd_do_prot_rw(struct se_cmd *cmd, struct fd_prot *fd_prot, if (is_write || ret 0) { kfree(fd_prot-prot_sg); - vfree(fd_prot-prot_buf); + kfree(fd_prot-prot_buf); } return ret; @@ -658,11 +659,11 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, 0, fd_prot.prot_sg, 0); if (rc) { kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); return rc; } kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); } } else { memset(fd_prot, 0, sizeof(struct fd_prot)); @@ -678,7 +679,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, 0, fd_prot.prot_sg, 0); if (rc) { kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); return rc; } } @@ -714,7 +715,7 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, if (ret 0) { kfree(fd_prot.prot_sg); - vfree(fd_prot.prot_buf); + kfree(fd_prot.prot_buf); return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; } -- 1.9.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 -next v3] target/rd: Don't pass imcomplete scatterlist entries to sbc_dif_verify_*
The scatterlist for protection information which is passed to sbc_dif_verify_read() or sbc_dif_verify_write() requires that neighboring scatterlist entries are contiguous or chained so that they can be iterated by sg_next(). However, the protection information for RD-MCP backends could be located in the multiple scatterlist arrays when the ramdisk space is too large. So if the read/write request straddles this boundary, sbc_dif_verify_read() or sbc_dif_verify_write() can't iterate all scatterlist entries. This problem can be fixed by chaining protection information scatterlist at creation time. For the architectures which don't support sg chaining (i.e. !CONFIG_ARCH_HAS_SG_CHAIN), fix it by allocating temporary scatterlist if needed. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@dev.mellanox.co.il Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * v3 - Fix it by chaining protection information scatterlist at creation time if CONFIG_ARCH_HAS_SG_CHAIN is defined. drivers/target/target_core_rd.c | 67 +++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index ac5e8d2..b4e6de0 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -139,10 +139,22 @@ static int rd_allocate_sgl_table(struct rd_dev *rd_dev, struct rd_dev_sg_table * unsigned char *p; while (total_sg_needed) { + unsigned int chain_entry = 0; + sg_per_table = (total_sg_needed max_sg_per_table) ? max_sg_per_table : total_sg_needed; - sg = kzalloc(sg_per_table * sizeof(struct scatterlist), +#ifdef CONFIG_ARCH_HAS_SG_CHAIN + + /* +* Reserve extra element for chain entry +*/ + if (sg_per_table total_sg_needed) + chain_entry = 1; + +#endif /* CONFIG_ARCH_HAS_SG_CHAIN */ + + sg = kcalloc(sg_per_table + chain_entry, sizeof(*sg), GFP_KERNEL); if (!sg) { pr_err(Unable to allocate scatterlist array @@ -150,7 +162,16 @@ static int rd_allocate_sgl_table(struct rd_dev *rd_dev, struct rd_dev_sg_table * return -ENOMEM; } - sg_init_table(sg, sg_per_table); + sg_init_table(sg, sg_per_table + chain_entry); + +#ifdef CONFIG_ARCH_HAS_SG_CHAIN + + if (i 0) { + sg_chain(sg_table[i - 1].sg_table, +max_sg_per_table + 1, sg); + } + +#endif /* CONFIG_ARCH_HAS_SG_CHAIN */ sg_table[i].sg_table = sg; sg_table[i].rd_sg_count = sg_per_table; @@ -390,11 +411,13 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) struct se_device *se_dev = cmd-se_dev; struct rd_dev *dev = RD_DEV(se_dev); struct rd_dev_sg_table *prot_table; + bool need_to_release = false; struct scatterlist *prot_sg; u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; u32 prot_offset, prot_page; + u32 prot_npages __maybe_unused; u64 tmp; - sense_reason_t rc; + sense_reason_t rc = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; tmp = cmd-t_task_lba * se_dev-prot_length; prot_offset = do_div(tmp, PAGE_SIZE); @@ -407,7 +430,45 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) prot_sg = prot_table-sg_table[prot_page - prot_table-page_start_offset]; +#ifndef CONFIG_ARCH_HAS_SG_CHAIN + + prot_npages = DIV_ROUND_UP(prot_offset + sectors * se_dev-prot_length, + PAGE_SIZE); + + /* +* Allocate temporaly contiguous scatterlist entries if prot pages +* straddles multiple scatterlist tables. +*/ + if (prot_table-page_end_offset prot_page + prot_npages - 1) { + int i; + + prot_sg = kcalloc(prot_npages, sizeof(*prot_sg), GFP_KERNEL); + if (!prot_sg) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + need_to_release = true; + sg_init_table(prot_sg, prot_npages); + + for (i = 0; i prot_npages; i++) { + if (prot_page + i prot_table-page_end_offset) { + prot_table = rd_get_prot_table(dev, + prot_page + i); + if (!prot_table) { + kfree
Re: [PATCH v2 2/2] target/rd: Don't pass imcomplete scatterlist entries to sbc_dif_verify_*
2015-04-08 14:13 GMT+09:00 Nicholas A. Bellinger n...@linux-iscsi.org: On Sun, 2015-04-05 at 23:59 +0900, Akinobu Mita wrote: The scatterlist for protection information which is passed to sbc_dif_verify_read() or sbc_dif_verify_write() requires that neighboring scatterlist entries are contiguous or chained so that they can be iterated by sg_next(). However, the protection information for RD-MCP backends could be located in the multiple scatterlist arrays when the ramdisk space is too large. So if the read/write request straddles this boundary, sbc_dif_verify_read() or sbc_dif_verify_write() can't iterate all scatterlist entries. This fixes it by allocating temporary scatterlist if it is needed. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@dev.mellanox.co.il Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * No change from v1 drivers/target/target_core_rd.c | 39 +++ 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index ac5e8d2..6e25eaa 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -390,11 +390,12 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) struct se_device *se_dev = cmd-se_dev; struct rd_dev *dev = RD_DEV(se_dev); struct rd_dev_sg_table *prot_table; + bool need_to_release = false; struct scatterlist *prot_sg; u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; + u32 prot_offset, prot_page, prot_npages; u64 tmp; - sense_reason_t rc; + sense_reason_t rc = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; tmp = cmd-t_task_lba * se_dev-prot_length; prot_offset = do_div(tmp, PAGE_SIZE); @@ -404,10 +405,40 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) if (!prot_table) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - prot_sg = prot_table-sg_table[prot_page - - prot_table-page_start_offset]; + prot_npages = DIV_ROUND_UP(prot_offset + sectors * se_dev-prot_length, +PAGE_SIZE); + + /* prot pages straddles multiple scatterlist tables */ + if (prot_table-page_end_offset prot_page + prot_npages - 1) { + int i; + + prot_sg = kcalloc(prot_npages, sizeof(*prot_sg), GFP_KERNEL); + if (!prot_sg) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + need_to_release = true; + sg_init_table(prot_sg, prot_npages); + + for (i = 0; i prot_npages; i++) { + if (prot_page + i prot_table-page_end_offset) { + prot_table = rd_get_prot_table(dev, + prot_page + i); + if (!prot_table) + goto out; + sg_unmark_end(prot_sg[i - 1]); + } + prot_sg[i] = prot_table-sg_table[prot_page + i - + prot_table-page_start_offset]; + } + } else { + prot_sg = prot_table-sg_table[prot_page - + prot_table-page_start_offset]; + } Mmm, how about just explicitly doing sg_link() at prot_table scatterlist creation time instead..? Yeap, that would be an optimal solution. But some architectures don't support sg chaining (i.e. !CONFIG_ARCH_HAS_SG_CHAIN). That would save the extra allocation above, and AFAICT make for simpler code. Do you prefer fixing it conditionally with using #ifdef ? (i.e. sg chaining at creating time if CONFIG_ARCH_HAS_SG_CHAIN is defined, otherwise do the extra allocation above) If so, I'll resubmit the patch with such changes. -- 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 1/2] target/rd: reduce code duplication in rd_execute_rw()
2015-04-06 16:43 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/5/2015 5:59 PM, Akinobu Mita wrote: Factor out code duplication in rd_execute_rw() into a helper function rd_do_prot_rw(). This change is required to minimize the forthcoming fix in rd_do_prot_rw(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@dev.mellanox.co.il Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * v2 - Pass dif_verify() function pointer to helper function instead of is_write, suggested by Sagi Grimberg. drivers/target/target_core_rd.c | 66 - 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 98e83ac..ac5e8d2 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -382,6 +382,36 @@ static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page return NULL; } +typedef sense_reason_t (*dif_verify)(struct se_cmd *, sector_t, unsigned int, +unsigned int, struct scatterlist *, int); + +static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) +{ + struct se_device *se_dev = cmd-se_dev; + struct rd_dev *dev = RD_DEV(se_dev); + struct rd_dev_sg_table *prot_table; + struct scatterlist *prot_sg; + u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; + u32 prot_offset, prot_page; + u64 tmp; + sense_reason_t rc; + + tmp = cmd-t_task_lba * se_dev-prot_length; + prot_offset = do_div(tmp, PAGE_SIZE); + prot_page = tmp; + + prot_table = rd_get_prot_table(dev, prot_page); + if (!prot_table) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + prot_sg = prot_table-sg_table[prot_page - + prot_table-page_start_offset]; + + rc = dif_verify(cmd, cmd-t_task_lba, sectors, 0, prot_sg, prot_offset); + + return rc; Nit, Given there is no action on the returned rc, this can be reduced to: return dif_verify(); You are right, but the patch 2/2 inserts the lines to release temporary prot_sg before return statement, so assignment and return statements will be both required in the end. -- 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 2/2] target/rd: Don't pass imcomplete scatterlist entries to sbc_dif_verify_*
2015-04-05 19:10 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/4/2015 3:24 PM, Akinobu Mita wrote: The scatterlist for protection information which is passed to sbc_dif_verify_read() or sbc_dif_verify_write() requires that neighboring scatterlist entries are contiguous or chained so that they can be iterated by sg_next(). However, the protection information for RD-MCP backends could be located in the multiple scatterlist arrays when the ramdisk space is too large. So if the read/write request straddles this boundary, sbc_dif_verify_read() or sbc_dif_verify_write() can't iterate all scatterlist entries. This fixes it by allocating temporary scatterlist if it is needed. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Asias He as...@redhat.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_rd.c | 39 +++ 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 4d614c9..19c893d 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -387,11 +387,12 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, bool is_write) struct se_device *se_dev = cmd-se_dev; struct rd_dev *dev = RD_DEV(se_dev); struct rd_dev_sg_table *prot_table; + bool need_to_release = false; struct scatterlist *prot_sg; u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; + u32 prot_offset, prot_page, prot_npages; u64 tmp; - sense_reason_t rc; + sense_reason_t rc = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; sense_reason_t (*dif_verify)(struct se_cmd *, sector_t, unsigned int, unsigned int, struct scatterlist *, int) = is_write ? sbc_dif_verify_write : sbc_dif_verify_read; @@ -404,10 +405,40 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, bool is_write) if (!prot_table) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - prot_sg = prot_table-sg_table[prot_page - - prot_table-page_start_offset]; + prot_npages = DIV_ROUND_UP(prot_offset + sectors * se_dev-prot_length, + PAGE_SIZE); + + /* prot pages straddles multiple scatterlist tables */ + if (prot_table-page_end_offset prot_page + prot_npages - 1) { + int i; + + prot_sg = kcalloc(prot_npages, sizeof(*prot_sg), GFP_KERNEL); + if (!prot_sg) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + need_to_release = true; + sg_init_table(prot_sg, prot_npages); + + for (i = 0; i prot_npages; i++) { + if (prot_page + i prot_table-page_end_offset) { + prot_table = rd_get_prot_table(dev, + prot_page + i); + if (!prot_table) + goto out; + sg_unmark_end(prot_sg[i - 1]); + } + prot_sg[i] = prot_table-sg_table[prot_page + i - + prot_table-page_start_offset]; + } + } else { + prot_sg = prot_table-sg_table[prot_page - + prot_table-page_start_offset]; + } rc = dif_verify(cmd, cmd-t_task_lba, sectors, 0, prot_sg, prot_offset); +out: + if (need_to_release) + kfree(prot_sg); I think it is safe to free prot_sg if you just make sure to initialize it to NULL at declaration time, no need for 'need_to_release'. 'need_to_release' is required to avoid kfree when 'prot_sg' is initialized to a pointer in the real prot space (i.e. else case above). -- 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 v2 2/2] target/rd: Don't pass imcomplete scatterlist entries to sbc_dif_verify_*
The scatterlist for protection information which is passed to sbc_dif_verify_read() or sbc_dif_verify_write() requires that neighboring scatterlist entries are contiguous or chained so that they can be iterated by sg_next(). However, the protection information for RD-MCP backends could be located in the multiple scatterlist arrays when the ramdisk space is too large. So if the read/write request straddles this boundary, sbc_dif_verify_read() or sbc_dif_verify_write() can't iterate all scatterlist entries. This fixes it by allocating temporary scatterlist if it is needed. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@dev.mellanox.co.il Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * No change from v1 drivers/target/target_core_rd.c | 39 +++ 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index ac5e8d2..6e25eaa 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -390,11 +390,12 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) struct se_device *se_dev = cmd-se_dev; struct rd_dev *dev = RD_DEV(se_dev); struct rd_dev_sg_table *prot_table; + bool need_to_release = false; struct scatterlist *prot_sg; u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; + u32 prot_offset, prot_page, prot_npages; u64 tmp; - sense_reason_t rc; + sense_reason_t rc = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; tmp = cmd-t_task_lba * se_dev-prot_length; prot_offset = do_div(tmp, PAGE_SIZE); @@ -404,10 +405,40 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) if (!prot_table) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - prot_sg = prot_table-sg_table[prot_page - - prot_table-page_start_offset]; + prot_npages = DIV_ROUND_UP(prot_offset + sectors * se_dev-prot_length, + PAGE_SIZE); + + /* prot pages straddles multiple scatterlist tables */ + if (prot_table-page_end_offset prot_page + prot_npages - 1) { + int i; + + prot_sg = kcalloc(prot_npages, sizeof(*prot_sg), GFP_KERNEL); + if (!prot_sg) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + need_to_release = true; + sg_init_table(prot_sg, prot_npages); + + for (i = 0; i prot_npages; i++) { + if (prot_page + i prot_table-page_end_offset) { + prot_table = rd_get_prot_table(dev, + prot_page + i); + if (!prot_table) + goto out; + sg_unmark_end(prot_sg[i - 1]); + } + prot_sg[i] = prot_table-sg_table[prot_page + i - + prot_table-page_start_offset]; + } + } else { + prot_sg = prot_table-sg_table[prot_page - + prot_table-page_start_offset]; + } rc = dif_verify(cmd, cmd-t_task_lba, sectors, 0, prot_sg, prot_offset); +out: + if (need_to_release) + kfree(prot_sg); return rc; } -- 1.9.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 v2 1/2] target/rd: reduce code duplication in rd_execute_rw()
Factor out code duplication in rd_execute_rw() into a helper function rd_do_prot_rw(). This change is required to minimize the forthcoming fix in rd_do_prot_rw(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Sagi Grimberg sa...@dev.mellanox.co.il Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * v2 - Pass dif_verify() function pointer to helper function instead of is_write, suggested by Sagi Grimberg. drivers/target/target_core_rd.c | 66 - 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 98e83ac..ac5e8d2 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -382,6 +382,36 @@ static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page return NULL; } +typedef sense_reason_t (*dif_verify)(struct se_cmd *, sector_t, unsigned int, +unsigned int, struct scatterlist *, int); + +static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, dif_verify dif_verify) +{ + struct se_device *se_dev = cmd-se_dev; + struct rd_dev *dev = RD_DEV(se_dev); + struct rd_dev_sg_table *prot_table; + struct scatterlist *prot_sg; + u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; + u32 prot_offset, prot_page; + u64 tmp; + sense_reason_t rc; + + tmp = cmd-t_task_lba * se_dev-prot_length; + prot_offset = do_div(tmp, PAGE_SIZE); + prot_page = tmp; + + prot_table = rd_get_prot_table(dev, prot_page); + if (!prot_table) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + prot_sg = prot_table-sg_table[prot_page - + prot_table-page_start_offset]; + + rc = dif_verify(cmd, cmd-t_task_lba, sectors, 0, prot_sg, prot_offset); + + return rc; +} + static sense_reason_t rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, enum dma_data_direction data_direction) @@ -420,23 +450,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, cmd-t_task_lba, rd_size, rd_page, rd_offset); if (cmd-prot_type data_direction == DMA_TO_DEVICE) { - struct rd_dev_sg_table *prot_table; - struct scatterlist *prot_sg; - u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; - - tmp = cmd-t_task_lba * se_dev-prot_length; - prot_offset = do_div(tmp, PAGE_SIZE); - prot_page = tmp; - - prot_table = rd_get_prot_table(dev, prot_page); - if (!prot_table) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - - prot_sg = prot_table-sg_table[prot_page - prot_table-page_start_offset]; - - rc = sbc_dif_verify_write(cmd, cmd-t_task_lba, sectors, 0, - prot_sg, prot_offset); + rc = rd_do_prot_rw(cmd, sbc_dif_verify_write); if (rc) return rc; } @@ -503,23 +517,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, sg_miter_stop(m); if (cmd-prot_type data_direction == DMA_FROM_DEVICE) { - struct rd_dev_sg_table *prot_table; - struct scatterlist *prot_sg; - u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; - - tmp = cmd-t_task_lba * se_dev-prot_length; - prot_offset = do_div(tmp, PAGE_SIZE); - prot_page = tmp; - - prot_table = rd_get_prot_table(dev, prot_page); - if (!prot_table) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - - prot_sg = prot_table-sg_table[prot_page - prot_table-page_start_offset]; - - rc = sbc_dif_verify_read(cmd, cmd-t_task_lba, sectors, 0, -prot_sg, prot_offset); + rc = rd_do_prot_rw(cmd, sbc_dif_verify_read); if (rc) return rc; } -- 1.9.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/2] target/rd: reduce code duplication in rd_execute_rw()
2015-04-05 19:01 GMT+09:00 Sagi Grimberg sa...@dev.mellanox.co.il: On 4/4/2015 3:24 PM, Akinobu Mita wrote: Factor out code duplication in rd_execute_rw() into a helper function rd_do_prot_rw(). This change is required to minimize the forthcoming fix in rd_do_prot_rw(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Asias He as...@redhat.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_rd.c | 66 - 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 98e83ac..4d614c9 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -382,6 +382,36 @@ static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page return NULL; } +static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, bool is_write) +{ + struct se_device *se_dev = cmd-se_dev; + struct rd_dev *dev = RD_DEV(se_dev); + struct rd_dev_sg_table *prot_table; + struct scatterlist *prot_sg; + u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; + u32 prot_offset, prot_page; + u64 tmp; + sense_reason_t rc; + sense_reason_t (*dif_verify)(struct se_cmd *, sector_t, unsigned int, + unsigned int, struct scatterlist *, int) = + is_write ? sbc_dif_verify_write : sbc_dif_verify_read; Hi Akinobu, I think it would make more sense to pass the dif_verify() function pointer instead of is_write (only used for that anyway). Make sense. I'll update this series with that change. -- 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 2/2] target/rd: Don't pass imcomplete scatterlist entries to sbc_dif_verify_*
The scatterlist for protection information which is passed to sbc_dif_verify_read() or sbc_dif_verify_write() requires that neighboring scatterlist entries are contiguous or chained so that they can be iterated by sg_next(). However, the protection information for RD-MCP backends could be located in the multiple scatterlist arrays when the ramdisk space is too large. So if the read/write request straddles this boundary, sbc_dif_verify_read() or sbc_dif_verify_write() can't iterate all scatterlist entries. This fixes it by allocating temporary scatterlist if it is needed. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Asias He as...@redhat.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_rd.c | 39 +++ 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 4d614c9..19c893d 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -387,11 +387,12 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, bool is_write) struct se_device *se_dev = cmd-se_dev; struct rd_dev *dev = RD_DEV(se_dev); struct rd_dev_sg_table *prot_table; + bool need_to_release = false; struct scatterlist *prot_sg; u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; + u32 prot_offset, prot_page, prot_npages; u64 tmp; - sense_reason_t rc; + sense_reason_t rc = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; sense_reason_t (*dif_verify)(struct se_cmd *, sector_t, unsigned int, unsigned int, struct scatterlist *, int) = is_write ? sbc_dif_verify_write : sbc_dif_verify_read; @@ -404,10 +405,40 @@ static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, bool is_write) if (!prot_table) return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - prot_sg = prot_table-sg_table[prot_page - - prot_table-page_start_offset]; + prot_npages = DIV_ROUND_UP(prot_offset + sectors * se_dev-prot_length, + PAGE_SIZE); + + /* prot pages straddles multiple scatterlist tables */ + if (prot_table-page_end_offset prot_page + prot_npages - 1) { + int i; + + prot_sg = kcalloc(prot_npages, sizeof(*prot_sg), GFP_KERNEL); + if (!prot_sg) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + need_to_release = true; + sg_init_table(prot_sg, prot_npages); + + for (i = 0; i prot_npages; i++) { + if (prot_page + i prot_table-page_end_offset) { + prot_table = rd_get_prot_table(dev, + prot_page + i); + if (!prot_table) + goto out; + sg_unmark_end(prot_sg[i - 1]); + } + prot_sg[i] = prot_table-sg_table[prot_page + i - + prot_table-page_start_offset]; + } + } else { + prot_sg = prot_table-sg_table[prot_page - + prot_table-page_start_offset]; + } rc = dif_verify(cmd, cmd-t_task_lba, sectors, 0, prot_sg, prot_offset); +out: + if (need_to_release) + kfree(prot_sg); return rc; } -- 1.9.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/2] target/rd: reduce code duplication in rd_execute_rw()
Factor out code duplication in rd_execute_rw() into a helper function rd_do_prot_rw(). This change is required to minimize the forthcoming fix in rd_do_prot_rw(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Nicholas Bellinger n...@linux-iscsi.org Cc: Asias He as...@redhat.com Cc: Martin K. Petersen martin.peter...@oracle.com Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley james.bottom...@hansenpartnership.com Cc: target-de...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/target/target_core_rd.c | 66 - 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c index 98e83ac..4d614c9 100644 --- a/drivers/target/target_core_rd.c +++ b/drivers/target/target_core_rd.c @@ -382,6 +382,36 @@ static struct rd_dev_sg_table *rd_get_prot_table(struct rd_dev *rd_dev, u32 page return NULL; } +static sense_reason_t rd_do_prot_rw(struct se_cmd *cmd, bool is_write) +{ + struct se_device *se_dev = cmd-se_dev; + struct rd_dev *dev = RD_DEV(se_dev); + struct rd_dev_sg_table *prot_table; + struct scatterlist *prot_sg; + u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; + u32 prot_offset, prot_page; + u64 tmp; + sense_reason_t rc; + sense_reason_t (*dif_verify)(struct se_cmd *, sector_t, unsigned int, + unsigned int, struct scatterlist *, int) = + is_write ? sbc_dif_verify_write : sbc_dif_verify_read; + + tmp = cmd-t_task_lba * se_dev-prot_length; + prot_offset = do_div(tmp, PAGE_SIZE); + prot_page = tmp; + + prot_table = rd_get_prot_table(dev, prot_page); + if (!prot_table) + return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; + + prot_sg = prot_table-sg_table[prot_page - + prot_table-page_start_offset]; + + rc = dif_verify(cmd, cmd-t_task_lba, sectors, 0, prot_sg, prot_offset); + + return rc; +} + static sense_reason_t rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, enum dma_data_direction data_direction) @@ -420,23 +450,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, cmd-t_task_lba, rd_size, rd_page, rd_offset); if (cmd-prot_type data_direction == DMA_TO_DEVICE) { - struct rd_dev_sg_table *prot_table; - struct scatterlist *prot_sg; - u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; - - tmp = cmd-t_task_lba * se_dev-prot_length; - prot_offset = do_div(tmp, PAGE_SIZE); - prot_page = tmp; - - prot_table = rd_get_prot_table(dev, prot_page); - if (!prot_table) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - - prot_sg = prot_table-sg_table[prot_page - prot_table-page_start_offset]; - - rc = sbc_dif_verify_write(cmd, cmd-t_task_lba, sectors, 0, - prot_sg, prot_offset); + rc = rd_do_prot_rw(cmd, true); if (rc) return rc; } @@ -503,23 +517,7 @@ rd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, sg_miter_stop(m); if (cmd-prot_type data_direction == DMA_FROM_DEVICE) { - struct rd_dev_sg_table *prot_table; - struct scatterlist *prot_sg; - u32 sectors = cmd-data_length / se_dev-dev_attrib.block_size; - u32 prot_offset, prot_page; - - tmp = cmd-t_task_lba * se_dev-prot_length; - prot_offset = do_div(tmp, PAGE_SIZE); - prot_page = tmp; - - prot_table = rd_get_prot_table(dev, prot_page); - if (!prot_table) - return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE; - - prot_sg = prot_table-sg_table[prot_page - prot_table-page_start_offset]; - - rc = sbc_dif_verify_read(cmd, cmd-t_task_lba, sectors, 0, -prot_sg, prot_offset); + rc = rd_do_prot_rw(cmd, false); if (rc) return rc; } -- 1.9.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 0/3] scsi: ufs: fix several issues caused by driver reloading
This patch set addresses several issues caused by driver reloading in ufs driver, although the first patch also fixes error path in driver probe. Akinobu Mita (3): scsi: ufs: avoid using hostdata after scsi_host_put() scsi: ufs: fix unbalanced power.usage_count after reloading driver scsi: ufs: fix unbalanced power.disable_depth after reloading driver drivers/scsi/ufs/ufshcd-pltfrm.c | 5 + drivers/scsi/ufs/ufshcd.c| 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) -- 1.9.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/3] scsi: ufs: avoid using hostdata after scsi_host_put()
The hostdata array, which is denoted by 'hba' in ufs driver, should not be accessed after calling scsi_host_put(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Hannes Reinecke h...@suse.de Cc: Sahitya Tummala stumm...@codeaurora.org Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 5d60a86..4e4de32 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -5239,12 +5239,12 @@ void ufshcd_remove(struct ufs_hba *hba) ufshcd_disable_intr(hba, hba-intr_mask); ufshcd_hba_stop(hba); - scsi_host_put(hba-host); - ufshcd_exit_clk_gating(hba); if (ufshcd_is_clkscaling_enabled(hba)) devfreq_remove_device(hba-devfreq); ufshcd_hba_exit(hba); + + scsi_host_put(hba-host); } EXPORT_SYMBOL_GPL(ufshcd_remove); @@ -5547,9 +5547,9 @@ exit_gating: ufshcd_exit_clk_gating(hba); out_disable: hba-is_irq_enabled = false; - scsi_host_put(host); ufshcd_hba_exit(hba); out_error: + scsi_host_put(host); return err; } EXPORT_SYMBOL_GPL(ufshcd_init); -- 1.9.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 3/3] scsi: ufs: fix unbalanced power.disable_depth after reloading driver
Every time the driver is reloaded, the warning message Unbalanced pm_runtime_enable! is triggered due to unbalanced power.disable_depth. This is because pm_runtime_enable() is called during driver probe but pm_runtime_disable() is missed on driver remove. This also restores the device's runtime PM status to 'suspended' on driver remove as it was set to 'active' during driver probe. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Maya Erez me...@codeaurora.org Cc: Raviv Shvili rshv...@codeaurora.org Cc: Sahitya Tummala stumm...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Rafael J. Wysocki rafael.j.wyso...@intel.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd-pltfrm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index 4164108..b91cf09c 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -369,6 +369,10 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev) pm_runtime_get_sync((pdev)-dev); ufshcd_remove(hba); pm_runtime_put_sync(pdev-dev); + + pm_runtime_disable(pdev-dev); + pm_runtime_set_suspended(pdev-dev); + return 0; } -- 1.9.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 2/3] scsi: ufs: fix unbalanced power.usage_count after reloading driver
On driver removal, pm_runtime_get_sync() is called, but pm_runtime_put_sync() is missed. So once the driver is reloaded, the device's power.usage_count is unbalanced and the idle callback for the device will never be called. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Christoph Hellwig h...@lst.de Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Maya Erez me...@codeaurora.org Cc: Sahitya Tummala stumm...@codeaurora.org Cc: Rafael J. Wysocki rafael.j.wyso...@intel.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/ufs/ufshcd-pltfrm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index 7db9564..4164108 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -368,6 +368,7 @@ static int ufshcd_pltfrm_remove(struct platform_device *pdev) pm_runtime_get_sync((pdev)-dev); ufshcd_remove(hba); + pm_runtime_put_sync(pdev-dev); return 0; } -- 1.9.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] hpsa: cleanup bitops usage
Remove unnecessary adjustment for bit number and address of bitmask, and use BITS_TO_LONGS macro to calculate bitmap size. This change just simplifies the code a bit. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Don Brace don.br...@pmcs.com Cc: iss_storage...@hp.com Cc: storage...@pmcs.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org --- drivers/scsi/hpsa.c | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index a1cfbd3..73a282b 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -4697,8 +4697,7 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h) offset = (i + 1) % h-nr_cmds; continue; } - set_bit(i (BITS_PER_LONG - 1), - h-cmd_pool_bits + (i / BITS_PER_LONG)); + set_bit(i, h-cmd_pool_bits); break; /* it's ours now. */ } h-last_allocation = i; /* benignly racy */ @@ -4729,8 +4728,7 @@ static void cmd_free(struct ctlr_info *h, struct CommandList *c) int i; i = c - h-cmd_pool; - clear_bit(i (BITS_PER_LONG - 1), - h-cmd_pool_bits + (i / BITS_PER_LONG)); + clear_bit(i, h-cmd_pool_bits); } } @@ -6410,8 +6408,7 @@ out_disable: static int hpsa_allocate_cmd_pool(struct ctlr_info *h) { - h-cmd_pool_bits = kzalloc( - DIV_ROUND_UP(h-nr_cmds, BITS_PER_LONG) * + h-cmd_pool_bits = kcalloc(BITS_TO_LONGS(h-nr_cmds), sizeof(unsigned long), GFP_KERNEL); h-cmd_pool = pci_alloc_consistent(h-pdev, h-nr_cmds * sizeof(*h-cmd_pool), -- 1.9.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 v6 1/3] scsi: ufs: add ioctl interface for query request
2015-03-12 21:27 GMT+09:00 Gilad Broner gbro...@codeaurora.org: From: Dolev Raviv dra...@codeaurora.org This patch exposes the ioctl interface for UFS driver via SCSI device ioctl interface. As of now UFS driver would provide the ioctl for query interface to connected UFS device. Signed-off-by: Dolev Raviv dra...@codeaurora.org Signed-off-by: Noa Rubens n...@codeaurora.org Signed-off-by: Raviv Shvili rshv...@codeaurora.org Signed-off-by: Yaniv Gardi yga...@codeaurora.org Sorry to bother you again. Could you read two comments below? +/** + * ufshcd_ioctl - ufs ioctl callback registered in scsi_host + * @dev: scsi device required for per LUN queries + * @cmd: command opcode + * @buffer: user space buffer for transferring data + * + * Supported commands: + * UFS_IOCTL_QUERY + */ +static int ufshcd_ioctl(struct scsi_device *dev, int cmd, void __user *buffer) +{ + struct ufs_hba *hba = shost_priv(dev-host); + int err = 0; + + BUG_ON(!hba); + if (!buffer) { + dev_err(hba-dev, %s: User buffer is NULL!\n, __func__); + return -EINVAL; + } + Should we remove this check or move it into ufshcd_query_ioctl()? For example, BLKFLS ioctl without argument is correct usage, but it always triggers this message. (blkdev_ioctl - __blkdev_driver_ioctl - sd_ioctl - scsi_ioctl - ufshcd_ioctl) + switch (cmd) { + case UFS_IOCTL_QUERY: + pm_runtime_get_sync(hba-dev); + err = ufshcd_query_ioctl(hba, ufshcd_scsi_to_upiu_lun(dev-lun), + buffer); + pm_runtime_put_sync(hba-dev); + break; + case UFS_IOCTL_BLKROSET: + err = -ENOIOCTLCMD; + break; + default: + err = -EINVAL; + dev_err(hba-dev, %s: Illegal ufs-IOCTL cmd %d\n, __func__, + cmd); + break; + } + + return err; +} + static struct scsi_host_template ufshcd_driver_template = { .module = THIS_MODULE, .name = UFSHCD, @@ -4213,6 +4433,7 @@ static struct scsi_host_template ufshcd_driver_template = { .eh_abort_handler = ufshcd_abort, .eh_device_reset_handler = ufshcd_eh_device_reset_handler, .eh_host_reset_handler = ufshcd_eh_host_reset_handler, + .ioctl = ufshcd_ioctl, .this_id= -1, .sg_tablesize = SG_ALL, .cmd_per_lun= UFSHCD_CMD_PER_LUN, ... diff --git a/include/uapi/scsi/ufs/ioctl.h b/include/uapi/scsi/ufs/ioctl.h new file mode 100644 index 000..bc4eed7 --- /dev/null +++ b/include/uapi/scsi/ufs/ioctl.h @@ -0,0 +1,57 @@ +#ifndef UAPI_UFS_IOCTL_H_ +#define UAPI_UFS_IOCTL_H_ + +#include linux/types.h + +/* + * IOCTL opcode for ufs queries has the following opcode after + * SCSI_IOCTL_GET_PCI + */ +#define UFS_IOCTL_QUERY0x5388 Should we also need some comments near SCSI_IOCTL_GET_PCI in include/scsi/scsi.h in order to avoid someone trying to define the same ioctl code in the future? -- 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 v4 4/4] scsi: ufs: inject errors to verify error handling
2015-03-10 19:20 GMT+09:00 Gilad Broner gbro...@codeaurora.org: +static bool inject_cmd_hang_tr(struct ufs_hba *hba) +{ + int tag; + + tag = find_first_bit(hba-outstanding_reqs, hba-nutrs); + if (tag == hba-nutrs) + return 0; + + __clear_bit(tag, hba-outstanding_reqs); + hba-lrb[tag].cmd = NULL; + __clear_bit(tag, hba-lrb_in_use); hba-lrb_in_use is a bitmap set by test_and_set_bit_lock(). So this should be cleared by clear_bit_unlock(). You are correct. Thanks. And as soon as the bit corresponds to this slot in hba-lrb_in_use is cleared, this slot could be reused. But if the request corresponds to the slot is not completed yet, it could sacrifice the new request, too. So should we only inject into the commands which have been completed like this? Please note that we only clear the bit in hba-lrb_in_use. scsi_done is not called for this request. Therefore, the tag is not yet free in the block layer and next calls for queuecommand will not pass down this tag to be used in the UFS driver. So there is no danger of a new request being sacrificed. OK, I see there is no danger as far as the commands are comming through queuecommand(). But what about the query requests? PATCH 1/4 in this series has added ioctl interface for query request which also acquires a tag in hba-lrb_in_use through ufshcd_get_dev_cmd_tag(). Although it is very difficult to happen, is it possible for new query requests to be clashed by inject_cmd_hang_tr() in the same way I described earlier? -- 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 v4 4/4] scsi: ufs: inject errors to verify error handling
2015-03-02 23:56 GMT+09:00 Gilad Broner gbro...@codeaurora.org: From: Sujit Reddy Thumma sthu...@codeaurora.org Use fault-injection framework to simulate error conditions in the controller and verify error handling mechanisms implemented in UFS host controller driver. This is used only during development and hence guarded by CONFIG_UFS_FAULT_INJECTION debug config option. This feature looks useful, but I have a couple of comments and question. +static bool inject_cmd_hang_tr(struct ufs_hba *hba) +{ + int tag; + + tag = find_first_bit(hba-outstanding_reqs, hba-nutrs); + if (tag == hba-nutrs) + return 0; + + __clear_bit(tag, hba-outstanding_reqs); + hba-lrb[tag].cmd = NULL; + __clear_bit(tag, hba-lrb_in_use); hba-lrb_in_use is a bitmap set by test_and_set_bit_lock(). So this should be cleared by clear_bit_unlock(). And as soon as the bit corresponds to this slot in hba-lrb_in_use is cleared, this slot could be reused. But if the request corresponds to the slot is not completed yet, it could sacrifice the new request, too. So should we only inject into the commands which have been completed like this? tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); completed_reqs = tr_doorbell ^ hba-outstanding_reqs; tag = find_first_bit(completed_reqs, hba-nutrs); ... Or clear the corresponding bit in REG_UTP_TASK_REQ_LIST_CLEAR, just like what inject_fatal_err_tr() does? + + /* command hang injected */ + return 1; +} + +static int inject_cmd_hang_tm(struct ufs_hba *hba) +{ + int tag; + + tag = find_first_bit(hba-outstanding_tasks, hba-nutmrs); + if (tag == hba-nutmrs) + return 0; + + __clear_bit(tag, hba-outstanding_tasks); + __clear_bit(tag, hba-tm_slots_in_use); + + /* command hang injected */ + return 1; +} The same goes for hba-tm_slots_in_use. -- 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 v5 1/4] scsi: add ability to adjust module reference for scsi host
While accessing a scsi_device, the use count of the underlying LLDD module is incremented. The module reference is retrieved through .module field of struct scsi_host_template. This mapping between scsi_device and underlying LLDD module works well except ufs, unusual usb storage drivers, and sub drivers for esp_scsi. These drivers consist with core driver and actual LLDDs, and scsi_host_template is defined in the core driver. So the actual LLDDs can be unloaded even if the scsi_device is being accessed. This adds .module field in struct Scsi_Host and let the module reference be retrieved though it instead of struct scsi_host_template. This allows the actual LLDDs adjust module reference. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Reviewed-by: Hannes Reinecke h...@suse.de Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Matthew Dharm mdharm-...@one-eyed-alien.net Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Alan Stern st...@rowland.harvard.edu Cc: David S. Miller da...@davemloft.net Cc: Hannes Reinecke h...@suse.de Cc: linux-...@vger.kernel.org Cc: usb-stor...@lists.one-eyed-alien.net Cc: linux-scsi@vger.kernel.org --- Change from v3 - Rebased to the 4.0-rc1 drivers/scsi/hosts.c | 1 + drivers/scsi/scsi.c | 4 ++-- include/scsi/scsi_host.h | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index 8bb173e..21f1442 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -411,6 +411,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) */ shost-max_cmd_len = 12; shost-hostt = sht; + shost-module = sht-module; shost-this_id = sht-this_id; shost-can_queue = sht-can_queue; shost-sg_tablesize = sht-sg_tablesize; diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index c9c3b57..0d89344 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -981,7 +981,7 @@ int scsi_device_get(struct scsi_device *sdev) return -ENXIO; /* We can fail try_module_get if we're doing SCSI operations * from module exit (like cache flush) */ - __module_get(sdev-host-hostt-module); + __module_get(sdev-host-module); return 0; } @@ -997,7 +997,7 @@ EXPORT_SYMBOL(scsi_device_get); */ void scsi_device_put(struct scsi_device *sdev) { - module_put(sdev-host-hostt-module); + module_put(sdev-host-module); put_device(sdev-sdev_gendev); } EXPORT_SYMBOL(scsi_device_put); diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index e113c75..8742bfd 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -620,6 +620,7 @@ struct Scsi_Host { */ unsigned short max_cmd_len; + struct module *module; int this_id; int can_queue; short cmd_per_lun; -- 1.9.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 v5 4/4] scsi: esp_scsi: adjust module reference for scsi host
While accessing a scsi device on host adapter supported by sub driver for the ESP chip (mac_esp, am53c974, sun_esp, jazz_esp, sun3x_esp), the module reference count is not incremented. Because these drivers allocate scsi hosts with scsi_esp_template defined in ESP SCSI driver core module. So these drivers always can be unloaded. This fixes it by passing correct module reference to newly introduced scsi_esp_host_alloc() so that .module field in struct Scsi_Host can be adjusted. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Acked-by: David S. Miller da...@davemloft.net Acked-by: Hannes Reinecke h...@suse.de Cc: David S. Miller da...@davemloft.net Cc: Hannes Reinecke h...@suse.de Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org --- No change from v3 drivers/scsi/am53c974.c | 3 +-- drivers/scsi/esp_scsi.c | 16 +--- drivers/scsi/esp_scsi.h | 11 +++ drivers/scsi/jazz_esp.c | 3 +-- drivers/scsi/mac_esp.c | 3 +-- drivers/scsi/sun3x_esp.c | 3 +-- drivers/scsi/sun_esp.c | 3 +-- 7 files changed, 25 insertions(+), 17 deletions(-) diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c index a6f5ee8..3b53f01 100644 --- a/drivers/scsi/am53c974.c +++ b/drivers/scsi/am53c974.c @@ -400,7 +400,6 @@ static void dc390_check_eeprom(struct esp *esp) static int pci_esp_probe_one(struct pci_dev *pdev, const struct pci_device_id *id) { - struct scsi_host_template *hostt = scsi_esp_template; int err = -ENODEV; struct Scsi_Host *shost; struct esp *esp; @@ -417,7 +416,7 @@ static int pci_esp_probe_one(struct pci_dev *pdev, goto fail_disable_device; } - shost = scsi_host_alloc(hostt, sizeof(struct esp)); + shost = scsi_esp_host_alloc(sizeof(struct esp)); if (!shost) { dev_printk(KERN_INFO, pdev-dev, failed to allocate scsi host\n); diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c index 065b25d..4484c90 100644 --- a/drivers/scsi/esp_scsi.c +++ b/drivers/scsi/esp_scsi.c @@ -2675,8 +2675,7 @@ static const char *esp_info(struct Scsi_Host *host) return esp; } -struct scsi_host_template scsi_esp_template = { - .module = THIS_MODULE, +static struct scsi_host_template scsi_esp_template = { .name = esp, .info = esp_info, .queuecommand = esp_queuecommand, @@ -2696,7 +2695,18 @@ struct scsi_host_template scsi_esp_template = { .skip_settle_delay = 1, .use_blk_tags = 1, }; -EXPORT_SYMBOL(scsi_esp_template); + +struct Scsi_Host *__scsi_esp_host_alloc(int privsize, struct module *owner) +{ + struct Scsi_Host *shost; + + shost = scsi_host_alloc(scsi_esp_template, privsize); + if (shost) + shost-module = owner; + + return shost; +} +EXPORT_SYMBOL(__scsi_esp_host_alloc); static void esp_get_signalling(struct Scsi_Host *host) { diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h index 84dcbe4..9309e08 100644 --- a/drivers/scsi/esp_scsi.h +++ b/drivers/scsi/esp_scsi.h @@ -544,9 +544,8 @@ struct esp { /* A front-end driver for the ESP chip should do the following in * it's device probe routine: - * 1) Allocate the host and private area using scsi_host_alloc() - *with size 'sizeof(struct esp)'. The first argument to - *scsi_host_alloc() should be scsi_esp_template. + * 1) Allocate the host and private area using scsi_esp_host_alloc() + *with size 'sizeof(struct esp)'. * 2) Set host-max_id as appropriate. * 3) Set esp-host to the scsi_host itself, and esp-dev *to the device object pointer. @@ -573,7 +572,11 @@ struct esp { * 13) Check scsi_esp_register() return value, release all resources * if an error was returned. */ -extern struct scsi_host_template scsi_esp_template; +extern struct Scsi_Host *__scsi_esp_host_alloc(int privsize, + struct module *owner); +#define scsi_esp_host_alloc(privsize) \ + __scsi_esp_host_alloc(privsize, THIS_MODULE) + extern int scsi_esp_register(struct esp *, struct device *); extern void scsi_esp_unregister(struct esp *); diff --git a/drivers/scsi/jazz_esp.c b/drivers/scsi/jazz_esp.c index 9aaa74e..9c268f7 100644 --- a/drivers/scsi/jazz_esp.c +++ b/drivers/scsi/jazz_esp.c @@ -131,13 +131,12 @@ static const struct esp_driver_ops jazz_esp_ops = { static int esp_jazz_probe(struct platform_device *dev) { - struct scsi_host_template *tpnt = scsi_esp_template; struct Scsi_Host *host; struct esp *esp; struct resource *res; int err; - host = scsi_host_alloc(tpnt, sizeof(struct esp)); + host = scsi_esp_host_alloc(sizeof(struct esp)); err = -ENOMEM; if (!host) diff --git a/drivers/scsi/mac_esp.c b
[PATCH v5 0/4] scsi: ufs ums-* esp_scsi: fix module reference counting
While accessing a scsi_device, the use count of the underlying LLDD module is incremented. The module reference is retrieved through .module field of struct scsi_host_template. This mapping between scsi_device and underlying LLDD module works well except ufs, unusual usb storage drivers, and sub drivers for esp_scsi. These drivers consist with core driver and actual LLDDs, and scsi_host_template is defined in the core driver. So the actual LLDDs can be unloaded even if the scsi_device is being accessed. This patch series first adds ability to adjust module reference for scsi host by LLDDs and then fixes actual LLDDs by adjusting module reference after scsi host allocation. * v5: - Discard v4 changes and restore to v3. Because v4 shows that moving owner module field from scsi_host_template to Scsi_Host requires a lot of changes and introduces complication to existing drivers which don't have the module reference mismatch issue. - Rebased to the 4.0-rc1 * v4: - Patch series is almost rewritten as module reference field in struct scsi_host_template has been unused anymore. So Acked-by: and Reviewed-by: tags that have been received are deleted. * v3: - Add fix for ESP SCSI drivers * v2: - Pass correct module reference to usb_stor_probe1() instead of touching all ums-* drivers, suggested by Alan Stern Akinobu Mita (4): scsi: add ability to adjust module reference for scsi host scsi: ufs: adjust module reference for scsi host usb: storage: adjust module reference for scsi host scsi: esp_scsi: adjust module reference for scsi host drivers/scsi/am53c974.c | 3 +-- drivers/scsi/esp_scsi.c | 16 +--- drivers/scsi/esp_scsi.h | 11 +++ drivers/scsi/hosts.c | 1 + drivers/scsi/jazz_esp.c | 3 +-- drivers/scsi/mac_esp.c | 3 +-- drivers/scsi/scsi.c | 4 ++-- drivers/scsi/sun3x_esp.c | 3 +-- drivers/scsi/sun_esp.c | 3 +-- drivers/scsi/ufs/ufshcd-pci.c| 1 + drivers/scsi/ufs/ufshcd-pltfrm.c | 1 + drivers/scsi/ufs/ufshcd.c| 1 - drivers/usb/storage/usb.c| 8 +--- drivers/usb/storage/usb.h| 7 +-- include/scsi/scsi_host.h | 1 + 15 files changed, 41 insertions(+), 25 deletions(-) Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: Matthew Dharm mdharm-...@one-eyed-alien.net Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Alan Stern st...@rowland.harvard.edu Cc: David S. Miller da...@davemloft.net Cc: Hannes Reinecke h...@suse.de Cc: linux-...@vger.kernel.org Cc: usb-stor...@lists.one-eyed-alien.net Cc: linux-scsi@vger.kernel.org -- 1.9.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 v5 3/4] usb: storage: adjust module reference for scsi host
While accessing a unusual usb storage (ums-alauda, ums-cypress, ...), the module reference count is not incremented. Because these drivers allocate scsi hosts with usb_stor_host_template defined in usb-storage module. So these drivers always can be unloaded. This fixes it by passing correct module reference to usb_stor_probe1() so that .module field in struct Scsi_Host can be adjusted. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Acked-by: Alan Stern st...@rowland.harvard.edu Cc: Matthew Dharm mdharm-...@one-eyed-alien.net Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Alan Stern st...@rowland.harvard.edu Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-...@vger.kernel.org Cc: usb-stor...@lists.one-eyed-alien.net Cc: linux-scsi@vger.kernel.org --- No change from v3 drivers/usb/storage/usb.c | 8 +--- drivers/usb/storage/usb.h | 7 +-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c index d468d02..3bb2558 100644 --- a/drivers/usb/storage/usb.c +++ b/drivers/usb/storage/usb.c @@ -911,10 +911,11 @@ static unsigned int usb_stor_sg_tablesize(struct usb_interface *intf) } /* First part of general USB mass-storage probing */ -int usb_stor_probe1(struct us_data **pus, +int __usb_stor_probe1(struct us_data **pus, struct usb_interface *intf, const struct usb_device_id *id, - struct us_unusual_dev *unusual_dev) + struct us_unusual_dev *unusual_dev, + struct module *owner) { struct Scsi_Host *host; struct us_data *us; @@ -937,6 +938,7 @@ int usb_stor_probe1(struct us_data **pus, */ host-max_cmd_len = 16; host-sg_tablesize = usb_stor_sg_tablesize(intf); + host-module = owner; *pus = us = host_to_us(host); mutex_init((us-dev_mutex)); us_set_lock_class(us-dev_mutex, intf); @@ -969,7 +971,7 @@ BadDevice: release_everything(us); return result; } -EXPORT_SYMBOL_GPL(usb_stor_probe1); +EXPORT_SYMBOL_GPL(__usb_stor_probe1); /* Second part of general USB mass-storage probing */ int usb_stor_probe2(struct us_data *us) diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h index 307e339..0cb74ba 100644 --- a/drivers/usb/storage/usb.h +++ b/drivers/usb/storage/usb.h @@ -194,10 +194,13 @@ extern int usb_stor_reset_resume(struct usb_interface *iface); extern int usb_stor_pre_reset(struct usb_interface *iface); extern int usb_stor_post_reset(struct usb_interface *iface); -extern int usb_stor_probe1(struct us_data **pus, +extern int __usb_stor_probe1(struct us_data **pus, struct usb_interface *intf, const struct usb_device_id *id, - struct us_unusual_dev *unusual_dev); + struct us_unusual_dev *unusual_dev, + struct module *owner); +#define usb_stor_probe1(pus, intf, id, unusual_dev) \ + __usb_stor_probe1(pus, intf, id, unusual_dev, THIS_MODULE) extern int usb_stor_probe2(struct us_data *us); extern void usb_stor_disconnect(struct usb_interface *intf); -- 1.9.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 v5 2/4] scsi: ufs: adjust module reference for scsi host
While accessing a UFS device, the module reference count for core driver (ufshcd) is incremented but not incremented for the actual glue driver (ufshcd-pci or ufshcd-pltfrm). Because these drivers allocate scsi hosts with scsi_host_template defined in ufshcd module. So these drivers always can be unloaded. This fixes it by adjusting module reference after scsi host allocation. Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Vinayak Holikatti vinholika...@gmail.com Cc: Dolev Raviv dra...@codeaurora.org Cc: Sujit Reddy Thumma sthu...@codeaurora.org Cc: Subhash Jadavani subha...@codeaurora.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org --- No change from v3 drivers/scsi/ufs/ufshcd-pci.c| 1 + drivers/scsi/ufs/ufshcd-pltfrm.c | 1 + drivers/scsi/ufs/ufshcd.c| 1 - 3 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c index 7062cc5..d98f072 100644 --- a/drivers/scsi/ufs/ufshcd-pci.c +++ b/drivers/scsi/ufs/ufshcd-pci.c @@ -142,6 +142,7 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) return err; } + hba-host-module = THIS_MODULE; INIT_LIST_HEAD(hba-clk_list_head); err = ufshcd_init(hba, mmio_base, pdev-irq); diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c index f9d0644..bd76a0c 100644 --- a/drivers/scsi/ufs/ufshcd-pltfrm.c +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c @@ -322,6 +322,7 @@ static int ufshcd_pltfrm_probe(struct platform_device *pdev) goto out; } + hba-host-module = THIS_MODULE; hba-vops = get_variant_ops(pdev-dev); err = ufshcd_parse_clock_info(hba); diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 8830aeb..87c4847 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -4200,7 +4200,6 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie) } static struct scsi_host_template ufshcd_driver_template = { - .module = THIS_MODULE, .name = UFSHCD, .proc_name = UFSHCD, .queuecommand = ufshcd_queuecommand, -- 1.9.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 v3 2/4] scsi: ufs: add debugfs for ufs
2015-02-23 17:08 GMT+09:00 Gilad Broner gbro...@codeaurora.org: From: Lee Susman lsus...@codeaurora.org Adding debugfs capability for ufshcd. debugfs attributes introduced in this patch: - View driver/controller runtime data - Command tag statistics for performance analisis - Dump device descriptor info - Track recoverable errors statistics during runtime - Change UFS power mode during runtime entry a string in the format 'GGLLMM' where: G - selected gear L - number of lanes M - power mode (1=fast mode, 2=slow mode, 4=fast-auto mode, 5=slow-auto mode) First letter is for RX, second is for TX. - Get/set DME attributes I have a few nitpick comments on this patch. +#ifdef CONFIG_DEBUG_FS + +#define UFSHCD_UPDATE_ERROR_STATS(hba, type) \ + do {\ + if (type UFS_ERR_MAX) \ + hba-ufs_stats.err_stats[type]++; \ + } while (0) + +#define UFSHCD_UPDATE_TAG_STATS(hba, tag) \ + do {\ + struct request *rq = hba-lrb[task_tag].cmd ? \ + hba-lrb[task_tag].cmd-request : NULL; \ + u64 **tag_stats = hba-ufs_stats.tag_stats; \ + int rq_type = -1; \ + if (!hba-ufs_stats.enabled)\ + break; \ + tag_stats[tag][TS_TAG]++; \ + if (!rq)\ + break; \ + WARN_ON(hba-ufs_stats.q_depth hba-nutrs); \ + if (rq_data_dir(rq) == READ)\ + rq_type = TS_READ; \ + else if (rq_data_dir(rq) == WRITE) \ + rq_type = TS_WRITE; \ + else if (rq-cmd_flags REQ_FLUSH) \ + rq_type = TS_FLUSH; \ + else\ + break; \ + tag_stats[hba-ufs_stats.q_depth++][rq_type]++; \ + } while (0) + +#define UFSHCD_UPDATE_TAG_STATS_COMPLETION(hba, cmd) \ + do {\ + struct request *rq = cmd ? cmd-request : NULL; \ + if (cmd-request \ + ((rq_data_dir(rq) == READ) || \ + (rq_data_dir(rq) == WRITE) || \ + (rq-cmd_flags REQ_FLUSH))) \ + hba-ufs_stats.q_depth--; \ + } while (0) + +#else +#define UFSHCD_UPDATE_TAG_STATS(hba, tag) +#define UFSHCD_UPDATE_TAG_STATS_COMPLETION(hba, cmd) +#define UFSHCD_UPDATE_ERROR_STATS(hba, type) + +#endif Is there any reason that these are defined as macros instead of static functions? @@ -5760,6 +5958,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) async_schedule(ufshcd_async_scan, hba); + ufsdbg_add_debugfs(hba); + return 0; out_remove_scsi_host: @@ -5769,6 +5969,7 @@ exit_gating: out_disable: hba-is_irq_enabled = false; scsi_host_put(host); + ufsdbg_remove_debugfs(hba); ufshcd_hba_exit(hba); out_error: return err; This ufsdbg_remove_debugfs() call on error path of ufshcd_init() is unnecessary. Because ufsdbg_add_debugfs() is called at the last of ufshcd_init() and can't fail. -- 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 2/4] scsi: ufs: add debugfs for ufs
2015-02-10 22:58 GMT+09:00 Gilad Broner gbro...@codeaurora.org: @@ -5760,6 +5963,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) async_schedule(ufshcd_async_scan, hba); + UFSDBG_ADD_DEBUGFS(hba); + return 0; out_remove_scsi_host: @@ -5769,6 +5974,7 @@ exit_gating: out_disable: hba-is_irq_enabled = false; scsi_host_put(host); + UFSDBG_REMOVE_DEBUGFS(hba); ufshcd_hba_exit(hba); out_error: return err; UFSDBG_REMOVE_DEBUGFS() is not called in the driver unloading path. So ufs debugfs directory is not removed when unloading driver. It should be called in ufshcd_remove(). BTW, do we really need UFSDBG_ADD_DEBUGFS() and UFSDBG_REMOVE_DEBUGFS() macros? We can use static inline functions in ufs-debugfs.h when !CONFIG_DEBUG_FS. #ifdef CONFIG_DEBUG_FS void ufsdbg_add_debugfs(struct ufs_hba *hba); void ufsdbg_remove_debugfs(struct ufs_hba *hba); #else static inline void ufsdbg_add_debugfs(struct ufs_hba *hba) { } static inline void ufsdbg_remove_debugfs(struct ufs_hba *hba) { } #endif -- 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 v2 1/2] ata: ahci_platform: fix owner module reference mismatch for scsi host
The owner module reference of the ahci platform's scsi_host is initialized to libahci_platform's one, because these drivers use a scsi_host_template defined in libahci_platform. So these drivers can be unloaded even if the scsi device is being accessed. This fixes it by pushing the scsi_host_template from libahci_platform to all leaf drivers. The scsi_host_template is passed through a new argument of ahci_platform_init_host(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Hans de Goede hdego...@redhat.com Cc: Tejun Heo t...@kernel.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Changes in v2 - use the specific platform driver name for each sht - add comments on top of ATA_BASE_SHT and AHCI_SHT drivers/ata/ahci.h | 4 drivers/ata/ahci_da850.c | 11 +-- drivers/ata/ahci_imx.c | 11 +-- drivers/ata/ahci_mvebu.c | 11 +-- drivers/ata/ahci_platform.c| 11 +-- drivers/ata/ahci_st.c | 11 +-- drivers/ata/ahci_sunxi.c | 11 +-- drivers/ata/ahci_tegra.c | 11 +-- drivers/ata/ahci_xgene.c | 11 +-- drivers/ata/libahci_platform.c | 10 -- include/linux/ahci_platform.h | 4 +++- include/linux/libata.h | 6 ++ 12 files changed, 89 insertions(+), 23 deletions(-) diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h index 40f0e34..c2d0b06 100644 --- a/drivers/ata/ahci.h +++ b/drivers/ata/ahci.h @@ -354,6 +354,10 @@ extern int ahci_ignore_sss; extern struct device_attribute *ahci_shost_attrs[]; extern struct device_attribute *ahci_sdev_attrs[]; +/* + * This must be instantiated by the edge drivers. Read the comments + * for ATA_BASE_SHT + */ #define AHCI_SHT(drv_name) \ ATA_NCQ_SHT(drv_name), \ .can_queue = AHCI_MAX_CMDS - 1,\ diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c index ce8a7a6..267a3d3 100644 --- a/drivers/ata/ahci_da850.c +++ b/drivers/ata/ahci_da850.c @@ -16,6 +16,8 @@ #include linux/ahci_platform.h #include ahci.h +#define DRV_NAME ahci_da850 + /* SATA PHY Control Register offset from AHCI base */ #define SATA_P0PHYCR_REG 0x178 @@ -59,6 +61,10 @@ static const struct ata_port_info ahci_da850_port_info = { .port_ops = ahci_platform_ops, }; +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(DRV_NAME), +}; + static int ahci_da850_probe(struct platform_device *pdev) { struct device *dev = pdev-dev; @@ -85,7 +91,8 @@ static int ahci_da850_probe(struct platform_device *pdev) da850_sata_init(dev, pwrdn_reg, hpriv-mmio); - rc = ahci_platform_init_host(pdev, hpriv, ahci_da850_port_info); + rc = ahci_platform_init_host(pdev, hpriv, ahci_da850_port_info, +ahci_platform_sht); if (rc) goto disable_resources; @@ -102,7 +109,7 @@ static struct platform_driver ahci_da850_driver = { .probe = ahci_da850_probe, .remove = ata_platform_remove_one, .driver = { - .name = ahci_da850, + .name = DRV_NAME, .pm = ahci_da850_pm_ops, }, }; diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 35d51c5..bd870ec 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -28,6 +28,8 @@ #include linux/libata.h #include ahci.h +#define DRV_NAME ahci-imx + enum { /* Timer 1-ms Register */ IMX_TIMER1MS= 0x00e0, @@ -524,6 +526,10 @@ static u32 imx_ahci_parse_props(struct device *dev, return reg_value; } +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(DRV_NAME), +}; + static int imx_ahci_probe(struct platform_device *pdev) { struct device *dev = pdev-dev; @@ -620,7 +626,8 @@ static int imx_ahci_probe(struct platform_device *pdev) reg_val = clk_get_rate(imxpriv-ahb_clk) / 1000; writel(reg_val, hpriv-mmio + IMX_TIMER1MS); - ret = ahci_platform_init_host(pdev, hpriv, ahci_imx_port_info); + ret = ahci_platform_init_host(pdev, hpriv, ahci_imx_port_info, + ahci_platform_sht); if (ret) goto disable_sata; @@ -678,7 +685,7 @@ static struct platform_driver imx_ahci_driver = { .probe = imx_ahci_probe, .remove = ata_platform_remove_one, .driver = { - .name = ahci-imx, + .name = DRV_NAME, .of_match_table = imx_ahci_of_match, .pm = ahci_imx_pm_ops, }, diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c index 64bb084..23716dd 100644 --- a/drivers/ata/ahci_mvebu.c +++ b/drivers/ata/ahci_mvebu.c @@ -19,6
[PATCH v2 2/2] ata: pata_platform: fix owner module reference mismatch for scsi host
The owner module reference of the pata_of_platform's scsi_host is initialized to pata_platform's one, because pata_of_platform driver use a scsi_host_template defined in pata_platform. So this drivers can be unloaded even if the scsi device is being accessed. This fixes it by propagating the scsi_host_template to pata_of_platform driver. The scsi_host_template is passed through a new argument of __pata_platform_probe(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Hans de Goede hdego...@redhat.com Cc: Tejun Heo t...@kernel.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- * Change in v2 - use the specific platform driver name for pata_of_platform sht drivers/ata/pata_of_platform.c | 10 -- drivers/ata/pata_platform.c| 8 +--- include/linux/ata_platform.h | 5 - 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c index dcc408a..b6b7af8 100644 --- a/drivers/ata/pata_of_platform.c +++ b/drivers/ata/pata_of_platform.c @@ -16,6 +16,12 @@ #include linux/ata_platform.h #include linux/libata.h +#define DRV_NAME pata_of_platform + +static struct scsi_host_template pata_platform_sht = { + ATA_PIO_SHT(DRV_NAME), +}; + static int pata_of_platform_probe(struct platform_device *ofdev) { int ret; @@ -63,7 +69,7 @@ static int pata_of_platform_probe(struct platform_device *ofdev) pio_mask |= (1 pio_mode) - 1; return __pata_platform_probe(ofdev-dev, io_res, ctl_res, irq_res, -reg_shift, pio_mask); +reg_shift, pio_mask, pata_platform_sht); } static struct of_device_id pata_of_platform_match[] = { @@ -74,7 +80,7 @@ MODULE_DEVICE_TABLE(of, pata_of_platform_match); static struct platform_driver pata_of_platform_driver = { .driver = { - .name = pata_of_platform, + .name = DRV_NAME, .of_match_table = pata_of_platform_match, }, .probe = pata_of_platform_probe, diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c index 1eedfe4..c503ded 100644 --- a/drivers/ata/pata_platform.c +++ b/drivers/ata/pata_platform.c @@ -78,6 +78,7 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr, * @irq_res: Resource representing IRQ and its flags * @ioport_shift: I/O port shift * @__pio_mask: PIO mask + * @sht: scsi_host_template to use when registering * * Register a platform bus IDE interface. Such interfaces are PIO and we * assume do not support IRQ sharing. @@ -99,7 +100,8 @@ static void pata_platform_setup_port(struct ata_ioports *ioaddr, */ int __pata_platform_probe(struct device *dev, struct resource *io_res, struct resource *ctl_res, struct resource *irq_res, - unsigned int ioport_shift, int __pio_mask) + unsigned int ioport_shift, int __pio_mask, + struct scsi_host_template *sht) { struct ata_host *host; struct ata_port *ap; @@ -170,7 +172,7 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res, /* activate */ return ata_host_activate(host, irq, irq ? ata_sff_interrupt : NULL, -irq_flags, pata_platform_sht); +irq_flags, sht); } EXPORT_SYMBOL_GPL(__pata_platform_probe); @@ -216,7 +218,7 @@ static int pata_platform_probe(struct platform_device *pdev) return __pata_platform_probe(pdev-dev, io_res, ctl_res, irq_res, pp_info ? pp_info-ioport_shift : 0, -pio_mask); +pio_mask, pata_platform_sht); } static struct platform_driver pata_platform_driver = { diff --git a/include/linux/ata_platform.h b/include/linux/ata_platform.h index 5c618a0..619d9e7 100644 --- a/include/linux/ata_platform.h +++ b/include/linux/ata_platform.h @@ -10,12 +10,15 @@ struct pata_platform_info { unsigned int ioport_shift; }; +struct scsi_host_template; + extern int __pata_platform_probe(struct device *dev, struct resource *io_res, struct resource *ctl_res, struct resource *irq_res, unsigned int ioport_shift, -int __pio_mask); +int __pio_mask, +struct scsi_host_template *sht); /* * Marvell SATA private data -- 1.9.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/2] ata: ahci_platform: fix owner module reference mismatch for scsi host
The owner module reference of the ahci platform's scsi_host is initialized to libahci_platform's one, because these drivers use a scsi_host_template defined in libahci_platform. So these drivers can be unloaded even if the scsi device is being accessed. This fixes it by pushing the scsi_host_template from libahci_platform to all leaf drivers. The scsi_host_template is passed through a new argument of ahci_platform_init_host(). Signed-off-by: Akinobu Mita akinobu.m...@gmail.com Cc: Hans de Goede hdego...@redhat.com Cc: Tejun Heo t...@kernel.org Cc: Christoph Hellwig h...@lst.de Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-...@vger.kernel.org Cc: linux-scsi@vger.kernel.org --- drivers/ata/ahci_da850.c | 7 ++- drivers/ata/ahci_imx.c | 7 ++- drivers/ata/ahci_mvebu.c | 7 ++- drivers/ata/ahci_platform.c| 7 ++- drivers/ata/ahci_st.c | 7 ++- drivers/ata/ahci_sunxi.c | 7 ++- drivers/ata/ahci_tegra.c | 7 ++- drivers/ata/ahci_xgene.c | 7 ++- drivers/ata/libahci_platform.c | 10 -- include/linux/ahci_platform.h | 4 +++- 10 files changed, 55 insertions(+), 15 deletions(-) diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c index ce8a7a6..26566d0a 100644 --- a/drivers/ata/ahci_da850.c +++ b/drivers/ata/ahci_da850.c @@ -59,6 +59,10 @@ static const struct ata_port_info ahci_da850_port_info = { .port_ops = ahci_platform_ops, }; +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(ahci_platform), +}; + static int ahci_da850_probe(struct platform_device *pdev) { struct device *dev = pdev-dev; @@ -85,7 +89,8 @@ static int ahci_da850_probe(struct platform_device *pdev) da850_sata_init(dev, pwrdn_reg, hpriv-mmio); - rc = ahci_platform_init_host(pdev, hpriv, ahci_da850_port_info); + rc = ahci_platform_init_host(pdev, hpriv, ahci_da850_port_info, +ahci_platform_sht); if (rc) goto disable_resources; diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 35d51c5..3916c3a 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -524,6 +524,10 @@ static u32 imx_ahci_parse_props(struct device *dev, return reg_value; } +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(ahci_platform), +}; + static int imx_ahci_probe(struct platform_device *pdev) { struct device *dev = pdev-dev; @@ -620,7 +624,8 @@ static int imx_ahci_probe(struct platform_device *pdev) reg_val = clk_get_rate(imxpriv-ahb_clk) / 1000; writel(reg_val, hpriv-mmio + IMX_TIMER1MS); - ret = ahci_platform_init_host(pdev, hpriv, ahci_imx_port_info); + ret = ahci_platform_init_host(pdev, hpriv, ahci_imx_port_info, + ahci_platform_sht); if (ret) goto disable_sata; diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c index 64bb084..af383cb 100644 --- a/drivers/ata/ahci_mvebu.c +++ b/drivers/ata/ahci_mvebu.c @@ -67,6 +67,10 @@ static const struct ata_port_info ahci_mvebu_port_info = { .port_ops = ahci_platform_ops, }; +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(ahci_platform), +}; + static int ahci_mvebu_probe(struct platform_device *pdev) { struct ahci_host_priv *hpriv; @@ -88,7 +92,8 @@ static int ahci_mvebu_probe(struct platform_device *pdev) ahci_mvebu_mbus_config(hpriv, dram); ahci_mvebu_regret_option(hpriv); - rc = ahci_platform_init_host(pdev, hpriv, ahci_mvebu_port_info); + rc = ahci_platform_init_host(pdev, hpriv, ahci_mvebu_port_info, +ahci_platform_sht); if (rc) goto disable_resources; diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c index 18d5398..6e63cbf 100644 --- a/drivers/ata/ahci_platform.c +++ b/drivers/ata/ahci_platform.c @@ -29,6 +29,10 @@ static const struct ata_port_info ahci_port_info = { .port_ops = ahci_platform_ops, }; +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(ahci_platform), +}; + static int ahci_probe(struct platform_device *pdev) { struct device *dev = pdev-dev; @@ -46,7 +50,8 @@ static int ahci_probe(struct platform_device *pdev) if (of_device_is_compatible(dev-of_node, hisilicon,hisi-ahci)) hpriv-flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ; - rc = ahci_platform_init_host(pdev, hpriv, ahci_port_info); + rc = ahci_platform_init_host(pdev, hpriv, ahci_port_info, +ahci_platform_sht); if (rc) goto disable_resources; diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c index 2f9e831..291bcb6 100644 --- a/drivers/ata/ahci_st.c +++ b/drivers/ata/ahci_st.c @@ -140,6 +140,10 @@ static const