[Bug 108771] scsi: ses: kasan: ses_enclosure_data_process use after free on boot SAS2X28
https://bugzilla.kernel.org/show_bug.cgi?id=108771 --- Comment #3 from Pavel Tikhomirov --- On 12/10/2015 03:43 AM, James Bottomley wrote: > On Wed, 2015-12-09 at 15:35 +0300, Pavel Tikhomirov wrote: >> >> On 12/08/2015 07:16 PM, James Bottomley wrote: >>> On Mon, 2015-12-07 at 14:01 +, bugzilla-dae...@bugzilla.kernel.org >>> wrote: https://bugzilla.kernel.org/show_bug.cgi?id=108771 --- Comment #1 from Pavel Tikhomirov --- Aditional info about enclosue(from that node, but older 3.10 based kernel): [root@p9 crash]# modprobe sg [root@p9 crash]# sg_map -i /dev/sg0 LSI SAS2X28 0e12 /dev/sg1 /dev/sda LSI MR9260-4i 2.13 [root@p9 crash]# lsscsi -gs [1:0:16:0] enclosu LSI SAS2X28 0e12 - /dev/sg0 - [1:2:0:0]diskLSI MR9260-4i2.13 /dev/sda /dev/sg1 3.99TB [root@p9 crash]# sg_ses /dev/sg0 LSI SAS2X28 0e12 Supported diagnostic pages: Supported Diagnostic Pages [sdp] [0x0] Configuration (SES) [cf] [0x1] Enclosure Status/Control (SES) [ec,es] [0x2] Element Descriptor (SES) [ed] [0x7] Additional Element Status (SES-2) [aes] [0xa] Download Microcode (SES-2) [dm] [0xe] [root@p9 crash]# sg_ses /dev/sg1 LSI MR9260-4i 2.13 disk device (not an enclosure) Supported diagnostic pages: >>> >>> OK, can you give us the contents of pages 1, 2 and 10 with >>> >>> sg_ses --page=1 --hex /dev/sg0 >>> sg_ses --page=2 --hex /dev/sg0 >>> sg_ses --page=10 --hex /dev/sg0 >>> >>> The version of the kernel you do this on doesn't really matter. >> >> Here are these pages: >> >> [root@p9 ~]# sg_ses --page=1 --hex /dev/sg0 >> LSI SAS2X28 0e12 >> Response in hex from diagnostic page: Configuration (SES) >>00 01 00 00 c9 00 00 00 00 11 00 09 2c 50 03 04 80 >> ...,P... >>10 00 a7 1e bf 4c 53 49 20 20 20 20 20 53 41 53 32LSI >>SAS2 >>20 58 32 38 20 20 20 20 20 20 20 20 20 30 65 31 32X28 >>0e12 >>30 11 22 33 44 55 00 00 00 17 0c 00 0b 04 01 00 13 >> ."3DU... >>40 03 03 00 04 12 02 00 0f 02 02 00 0e 0e 01 00 09 >> >>50 18 01 00 0d 19 0e 00 0e 11 02 00 0e 44 72 69 76 >> Driv >>60 65 20 53 6c 6f 74 73 54 65 6d 70 65 72 61 74 75e >> SlotsTemperatu >>70 72 65 20 53 65 6e 73 6f 72 73 46 61 6e 73 56 6fre >> SensorsFansVo >>80 6c 74 61 67 65 20 53 65 6e 73 6f 72 73 50 6f 77ltage >> SensorsPow >>90 65 72 20 53 75 70 70 6c 69 65 73 45 6e 63 6c 6fer >> SuppliesEnclo >>a0 73 75 72 65 53 41 53 20 45 78 70 61 6e 64 65 72sureSAS >> Expander >>b0 73 53 41 53 20 43 6f 6e 6e 65 63 74 6f 72 73 45sSAS >> ConnectorsE >>c0 74 68 65 72 6e 65 74 20 70 6f 72 74 73 thernet ports > > Wow, that's some crazy enclosure. The description says it's a single > primary subenclosure with 9 different element types comprising 12 Device > slots, 1 temperature sensor, 3 fans, 2 voltage sensors, 2 power > supplies, 1 Enclosure, 1 SAS Expander, 14 SAS connectors, 2 > Communications ports. For 38 total element descriptors > >> [root@p9 ~]# sg_ses --page=2 --hex /dev/sg0 >> LSI SAS2X28 0e12 >> Response in hex from diagnostic page: Enclosure Status (SES) >>00 02 00 00 c0 00 00 00 00 00 00 00 00 05 00 00 00 >> >>10 05 00 00 00 01 00 00 00 05 00 00 00 05 00 00 00 >> >>20 01 00 00 00 05 00 00 00 05 00 00 00 01 00 00 00 >> >>30 05 00 00 00 05 00 00 00 01 00 00 00 00 00 00 00 >> >>40 01 00 2c 00 00 00 00 00 05 00 00 50 05 00 00 50 >> ..,P...P >>50 05 00 00 50 00 00 00 00 01 00 01 f9 01 00 04 b3 >> ...P >>60 00 00 00 00 47 80 00 20 47 80 00 20 00 00 00 00G.. G.. >> >>70 01 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 >> >>80 01 11 ff 00 01 11 ff 00 01 20 00 00 01 20 00 00. >> ... .. >>90 01 20 00 00 01 20 00 00 01 20 00 00 01 20 00 00. ... ... >> ... .. >>a0 01 20 00 00 01 20 00 00 01 20 00 00 01 20 00 00. ... ... >> ... .. >>b0 01 20 00 00 01 20 00 00 00 00 00 00 00 00 00 00. ... >> .. >>c0 00 00 00 00 > > Given each type has one overall descriptor followed by the individual > ones, we have 38 + 9 = 47 total descriptors, which is what we see here. > >> [root@p9 ~]# sg_ses --page=10 --hex /dev/sg0 >> LSI SAS2X28 0e12 >> Response in hex from diagnostic page: Additional Element Status (SES-2) >>00 0a 00 01 fc 00 00 00 00 16 22 00 00 01 00 00 00 >> .".. >>10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> >>20 00 00 00 00 00 00 00 00 00 00 00 00
Re: [Bug 108771] scsi: ses: kasan: ses_enclosure_data_process use after free on boot SAS2X28
On 12/10/2015 03:43 AM, James Bottomley wrote: On Wed, 2015-12-09 at 15:35 +0300, Pavel Tikhomirov wrote: On 12/08/2015 07:16 PM, James Bottomley wrote: On Mon, 2015-12-07 at 14:01 +, bugzilla-dae...@bugzilla.kernel.org wrote: https://bugzilla.kernel.org/show_bug.cgi?id=108771 --- Comment #1 from Pavel Tikhomirov --- Aditional info about enclosue(from that node, but older 3.10 based kernel): [root@p9 crash]# modprobe sg [root@p9 crash]# sg_map -i /dev/sg0 LSI SAS2X28 0e12 /dev/sg1 /dev/sda LSI MR9260-4i 2.13 [root@p9 crash]# lsscsi -gs [1:0:16:0] enclosu LSI SAS2X28 0e12 - /dev/sg0 - [1:2:0:0]diskLSI MR9260-4i2.13 /dev/sda /dev/sg1 3.99TB [root@p9 crash]# sg_ses /dev/sg0 LSI SAS2X28 0e12 Supported diagnostic pages: Supported Diagnostic Pages [sdp] [0x0] Configuration (SES) [cf] [0x1] Enclosure Status/Control (SES) [ec,es] [0x2] Element Descriptor (SES) [ed] [0x7] Additional Element Status (SES-2) [aes] [0xa] Download Microcode (SES-2) [dm] [0xe] [root@p9 crash]# sg_ses /dev/sg1 LSI MR9260-4i 2.13 disk device (not an enclosure) Supported diagnostic pages: OK, can you give us the contents of pages 1, 2 and 10 with sg_ses --page=1 --hex /dev/sg0 sg_ses --page=2 --hex /dev/sg0 sg_ses --page=10 --hex /dev/sg0 The version of the kernel you do this on doesn't really matter. Here are these pages: [root@p9 ~]# sg_ses --page=1 --hex /dev/sg0 LSI SAS2X28 0e12 Response in hex from diagnostic page: Configuration (SES) 00 01 00 00 c9 00 00 00 00 11 00 09 2c 50 03 04 80 ...,P... 10 00 a7 1e bf 4c 53 49 20 20 20 20 20 53 41 53 32LSI SAS2 20 58 32 38 20 20 20 20 20 20 20 20 20 30 65 31 32X28 0e12 30 11 22 33 44 55 00 00 00 17 0c 00 0b 04 01 00 13 ."3DU... 40 03 03 00 04 12 02 00 0f 02 02 00 0e 0e 01 00 09 50 18 01 00 0d 19 0e 00 0e 11 02 00 0e 44 72 69 76 Driv 60 65 20 53 6c 6f 74 73 54 65 6d 70 65 72 61 74 75e SlotsTemperatu 70 72 65 20 53 65 6e 73 6f 72 73 46 61 6e 73 56 6fre SensorsFansVo 80 6c 74 61 67 65 20 53 65 6e 73 6f 72 73 50 6f 77ltage SensorsPow 90 65 72 20 53 75 70 70 6c 69 65 73 45 6e 63 6c 6fer SuppliesEnclo a0 73 75 72 65 53 41 53 20 45 78 70 61 6e 64 65 72sureSAS Expander b0 73 53 41 53 20 43 6f 6e 6e 65 63 74 6f 72 73 45sSAS ConnectorsE c0 74 68 65 72 6e 65 74 20 70 6f 72 74 73 thernet ports Wow, that's some crazy enclosure. The description says it's a single primary subenclosure with 9 different element types comprising 12 Device slots, 1 temperature sensor, 3 fans, 2 voltage sensors, 2 power supplies, 1 Enclosure, 1 SAS Expander, 14 SAS connectors, 2 Communications ports. For 38 total element descriptors [root@p9 ~]# sg_ses --page=2 --hex /dev/sg0 LSI SAS2X28 0e12 Response in hex from diagnostic page: Enclosure Status (SES) 00 02 00 00 c0 00 00 00 00 00 00 00 00 05 00 00 00 10 05 00 00 00 01 00 00 00 05 00 00 00 05 00 00 00 20 01 00 00 00 05 00 00 00 05 00 00 00 01 00 00 00 30 05 00 00 00 05 00 00 00 01 00 00 00 00 00 00 00 40 01 00 2c 00 00 00 00 00 05 00 00 50 05 00 00 50 ..,P...P 50 05 00 00 50 00 00 00 00 01 00 01 f9 01 00 04 b3 ...P 60 00 00 00 00 47 80 00 20 47 80 00 20 00 00 00 00G.. G.. 70 01 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 80 01 11 ff 00 01 11 ff 00 01 20 00 00 01 20 00 00. ... .. 90 01 20 00 00 01 20 00 00 01 20 00 00 01 20 00 00. ... ... ... .. a0 01 20 00 00 01 20 00 00 01 20 00 00 01 20 00 00. ... ... ... .. b0 01 20 00 00 01 20 00 00 00 00 00 00 00 00 00 00. ... .. c0 00 00 00 00 Given each type has one overall descriptor followed by the individual ones, we have 38 + 9 = 47 total descriptors, which is what we see here. [root@p9 ~]# sg_ses --page=10 --hex /dev/sg0 LSI SAS2X28 0e12 Response in hex from diagnostic page: Additional Element Status (SES-2) 00 0a 00 01 fc 00 00 00 00 16 22 00 00 01 00 00 00 .".. 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00 00 00 00 00 00 16 22 00 01 .".. 30 01 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50 16 22 00 02 01 00 00 02 00 00 00 01 50 03 04 80 ."..P... 60 00 a7 1e bf 50 03 04 80 00 a7 1e ae 00 00 00 00 P... 70 00 00 00 00 16 22 00 03 01 00 00 03 00 00 00 00 .".. 80 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ...
[PATCH] scsi_dh_alua: fix build warning
We were getting build warning about unused variables "vpd_pg83" and "d"
Fixes: 83ea0e5e3501 ("scsi_dh_alua: use scsi_vpd_tpg_id()")
Cc: Hannes Reinecke
Signed-off-by: Sudip Mukherjee
---
build warning with next-20151211 and build log is at:
https://travis-ci.org/sudipm-mukherjee/parport/jobs/96209206
---
drivers/scsi/device_handler/scsi_dh_alua.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
b/drivers/scsi/device_handler/scsi_dh_alua.c
index f100cbb..5a328bf 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -320,8 +320,6 @@ static int alua_check_tpgs(struct scsi_device *sdev)
*/
static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
{
- unsigned char *d;
- unsigned char __rcu *vpd_pg83;
int rel_port = -1, group_id;
group_id = scsi_vpd_tpg_id(sdev, &rel_port);
--
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] scsi_dh_alua: fix build warning
On 12/11/2015 09:36 AM, Sudip Mukherjee wrote:
We were getting build warning about unused variables "vpd_pg83" and "d"
Fixes: 83ea0e5e3501 ("scsi_dh_alua: use scsi_vpd_tpg_id()")
Cc: Hannes Reinecke
Signed-off-by: Sudip Mukherjee
---
build warning with next-20151211 and build log is at:
https://travis-ci.org/sudipm-mukherjee/parport/jobs/96209206
---
drivers/scsi/device_handler/scsi_dh_alua.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
b/drivers/scsi/device_handler/scsi_dh_alua.c
index f100cbb..5a328bf 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -320,8 +320,6 @@ static int alua_check_tpgs(struct scsi_device *sdev)
*/
static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
{
- unsigned char *d;
- unsigned char __rcu *vpd_pg83;
int rel_port = -1, group_id;
group_id = scsi_vpd_tpg_id(sdev, &rel_port);
A fix for this is already queued in Martin Petersens tree.
Cheers,
Hannes
--
Dr. Hannes ReineckezSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet
On Thu, Dec 10, 2015 at 04:14:17PM -0800, K. Y. Srinivasan wrote:
> The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in Linux
> match
> the Window's definition.
>
> Signed-off-by: K. Y. Srinivasan
> Reviewed-by: Long Li
> Tested-by: Alex Ng
> ---
> drivers/scsi/storvsc_drv.c |5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index c41f674..00bb4bd 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -92,9 +92,8 @@ enum vstor_packet_operation {
> */
>
> struct hv_fc_wwn_packet {
> - boolprimary_active;
> - u8 reserved1;
> - u8 reserved2;
> + u8 primary_active;
> + u8 reserved1[3];
> u8 primary_port_wwn[8];
> u8 primary_node_wwn[8];
> u8 secondary_port_wwn[8];
> --
> 1.7.4.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
Should this go through stable as well?
If yes:
Fixes: 8b612fa23 '[SCSI] storvsc: Update the storage protocol to win8 level'
Cc: sta...@vger.kernel.org # v3.11+
Reviewed-by: Johannes Thumshirn
--
Johannes Thumshirn Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
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/4] scsi: storvsc: Properly support Fibre Channel devices
On Thu, Dec 10, 2015 at 04:14:18PM -0800, K. Y. Srinivasan wrote:
> For FC devices managed by this driver, atttach the appropriate transport
> template. This will allow us to create the appropriate sysfs files for
> these devices. With this we can publish the wwn for both the port and the
> node.
>
> Signed-off-by: K. Y. Srinivasan
> Reviewed-by: Long Li
> Tested-by: Alex Ng
> ---
> drivers/scsi/storvsc_drv.c | 100 +--
> 1 files changed, 95 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 00bb4bd..b94d471 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -41,6 +41,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * All wire protocol details (storage protocol between the guest and the
> host)
> @@ -397,6 +398,7 @@ static int storvsc_timeout = 180;
>
> static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
>
> +static struct scsi_transport_template *fc_transport_template;
>
> static void storvsc_on_channel_callback(void *context);
>
> @@ -456,6 +458,11 @@ struct storvsc_device {
> /* Used for vsc/vsp channel reset process */
> struct storvsc_cmd_request init_request;
> struct storvsc_cmd_request reset_request;
> + /*
> + * Currently active port and node names for FC devices.
> + */
> + u64 node_name;
> + u64 port_name;
> };
>
> struct hv_host_device {
> @@ -695,7 +702,26 @@ static void handle_multichannel_storage(struct
> hv_device *device, int max_chns)
> vmbus_are_subchannels_present(device->channel);
> }
>
> -static int storvsc_channel_init(struct hv_device *device)
> +static void cache_wwn(struct storvsc_device *stor_device,
> + struct vstor_packet *vstor_packet)
> +{
> + /*
> + * Cache the currently active port and node ww names.
> + */
> + if (vstor_packet->wwn_packet.primary_active) {
> + stor_device->node_name =
> + wwn_to_u64(vstor_packet->wwn_packet.primary_node_wwn);
> + stor_device->port_name =
> + wwn_to_u64(vstor_packet->wwn_packet.primary_port_wwn);
> + } else {
> + stor_device->node_name =
> + wwn_to_u64(vstor_packet->wwn_packet.secondary_node_wwn);
> + stor_device->port_name =
> + wwn_to_u64(vstor_packet->wwn_packet.secondary_port_wwn);
> + }
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> {
> struct storvsc_device *stor_device;
> struct storvsc_cmd_request *request;
> @@ -837,6 +863,40 @@ static int storvsc_channel_init(struct hv_device *device)
> stor_device->max_transfer_bytes =
> vstor_packet->storage_channel_properties.max_transfer_bytes;
>
> + if (!is_fc)
> + goto done;
> +
> + memset(vstor_packet, 0, sizeof(struct vstor_packet));
> + vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
> + vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> +
> + ret = vmbus_sendpacket(device->channel, vstor_packet,
> +(sizeof(struct vstor_packet) -
> +vmscsi_size_delta),
> +(unsigned long)request,
> +VM_PKT_DATA_INBAND,
> +VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +
> + if (ret != 0)
> + goto cleanup;
> +
> + t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> + if (t == 0) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
> + }
> +
> + if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> + vstor_packet->status != 0)
> + goto cleanup;
> +
> + /*
> + * Cache the currently active port and node ww names.
> + */
> + cache_wwn(stor_device, vstor_packet);
> +
> +done:
> +
That goto use is a bit weird. I see you did it because of the 80 chars limit
but
if (is_fc) {
[...]
}
is way more readable IMHO.
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
> vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> @@ -1076,6 +1136,12 @@ static void storvsc_on_receive(struct hv_device
> *device,
> schedule_work(&work->work);
> break;
>
> + case VSTOR_OPERATION_FCHBA_DATA:
> + stor_device = get_in_stor_device(device);
> + cache_wwn(stor_device, vstor_packet);
> + fc_host_node_name(stor_device->host) = stor_device->node_name;
> + fc_host_port_name(stor_device->host) = stor_device->port_name;
> + break;
> default:
> break;
> }
> @@ -1131,7 +1197,8 @@ static void storvsc_on_channel_callback(void *context)
> return;
> }
>
> -static int storvsc_connect_to_vsp(struct
Re: [PATCH 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init()
On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote:
> The function storvsc_channel_init() repeatedly interacts with the host to
> extract various channel properties. Refactor this code to eliminate code
> repetition.
>
> Signed-off-by: K. Y. Srinivasan
> Reviewed-by: Long Li
> Tested-by: Alex Ng
> ---
> drivers/scsi/storvsc_drv.c | 155
>
> 1 files changed, 57 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index b94d471..6f18e94 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -721,29 +721,16 @@ static void cache_wwn(struct storvsc_device
> *stor_device,
> }
> }
>
> -static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +static int storvsc_execute_vstor_op(struct hv_device *device,
> + struct storvsc_cmd_request *request,
> + bool status_check)
> {
> - struct storvsc_device *stor_device;
> - struct storvsc_cmd_request *request;
> struct vstor_packet *vstor_packet;
> - int ret, t, i;
> - int max_chns;
> - bool process_sub_channels = false;
> -
> - stor_device = get_out_stor_device(device);
> - if (!stor_device)
> - return -ENODEV;
> + int ret, t;
>
> - request = &stor_device->init_request;
> vstor_packet = &request->vstor_packet;
>
> - /*
> - * Now, initiate the vsc/vsp initialization protocol on the open
> - * channel
> - */
> - memset(request, 0, sizeof(struct storvsc_cmd_request));
> init_completion(&request->wait_event);
> - vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>
> ret = vmbus_sendpacket(device->channel, vstor_packet,
> @@ -753,27 +740,62 @@ static int storvsc_channel_init(struct hv_device
> *device, bool is_fc)
> VM_PKT_DATA_INBAND,
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> if (ret != 0)
> - goto cleanup;
> + goto done;
>
> t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> if (t == 0) {
> ret = -ETIMEDOUT;
> - goto cleanup;
> + goto done;
> }
>
> + if (!status_check)
> + goto done;
> +
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> vstor_packet->status != 0) {
> ret = -EINVAL;
> - goto cleanup;
> + goto done;
> }
>
> +done:
> + return ret;
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +{
> + struct storvsc_device *stor_device;
> + struct storvsc_cmd_request *request;
> + struct vstor_packet *vstor_packet;
> + int ret, i;
> + int max_chns;
> + bool process_sub_channels = false;
> +
> + stor_device = get_out_stor_device(device);
> + if (!stor_device)
> + return -ENODEV;
> +
> + request = &stor_device->init_request;
> + vstor_packet = &request->vstor_packet;
> +
> + /*
> + * Now, initiate the vsc/vsp initialization protocol on the open
> + * channel
> + */
> + memset(request, 0, sizeof(struct storvsc_cmd_request));
> + vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> + ret = storvsc_execute_vstor_op(device, request, true);
> + if (ret)
> + goto cleanup;
> +
> + /*
> + * Query host supported protocol version.
> + */
>
> for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
> /* reuse the packet for version range supported */
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation =
> VSTOR_OPERATION_QUERY_PROTOCOL_VERSION;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>
> vstor_packet->version.major_minor =
> vmstor_protocols[i].protocol_version;
> @@ -783,20 +805,9 @@ static int storvsc_channel_init(struct hv_device
> *device, bool is_fc)
>*/
> vstor_packet->version.revision = 0;
>
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> -(sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> -(unsigned long)request,
> -VM_PKT_DATA_INBAND,
> -VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> - if (ret != 0)
> - goto cleanup;
> -
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0) {
> - ret = -ETIMEDOUT;
> + ret = storvsc_execute_vstor_op(device, request, false);
> + if (ret)
>
Re: [PATCH 4/4] scsi: storvsc: Tighten up the interrupt path
On Thu, Dec 10, 2015 at 04:14:20PM -0800, K. Y. Srinivasan wrote:
> On the interrupt path, we repeatedly establish the pointer to the
> storvsc_device. Fix this.
>
> Signed-off-by: K. Y. Srinivasan
> Reviewed-by: Long Li
> Tested-by: Alex Ng
> ---
> drivers/scsi/storvsc_drv.c | 23 ---
> 1 files changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 6f18e94..8ba9908 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -958,19 +958,16 @@ static void storvsc_handle_error(struct vmscsi_request
> *vm_srb,
> }
>
>
> -static void storvsc_command_completion(struct storvsc_cmd_request
> *cmd_request)
> +static void storvsc_command_completion(struct storvsc_cmd_request
> *cmd_request,
> +struct storvsc_device *stor_dev)
> {
> struct scsi_cmnd *scmnd = cmd_request->cmd;
> - struct hv_host_device *host_dev = shost_priv(scmnd->device->host);
> struct scsi_sense_hdr sense_hdr;
> struct vmscsi_request *vm_srb;
> struct Scsi_Host *host;
> - struct storvsc_device *stor_dev;
> - struct hv_device *dev = host_dev->dev;
> u32 payload_sz = cmd_request->payload_sz;
> void *payload = cmd_request->payload;
>
> - stor_dev = get_in_stor_device(dev);
> host = stor_dev->host;
>
> vm_srb = &cmd_request->vstor_packet.vm_srb;
> @@ -1000,14 +997,13 @@ static void storvsc_command_completion(struct
> storvsc_cmd_request *cmd_request)
> kfree(payload);
> }
>
> -static void storvsc_on_io_completion(struct hv_device *device,
> +static void storvsc_on_io_completion(struct storvsc_device *stor_device,
> struct vstor_packet *vstor_packet,
> struct storvsc_cmd_request *request)
> {
> - struct storvsc_device *stor_device;
> struct vstor_packet *stor_pkt;
> + struct hv_device *device = stor_device->device;
>
> - stor_device = hv_get_drvdata(device);
> stor_pkt = &request->vstor_packet;
>
> /*
> @@ -1062,7 +1058,7 @@ static void storvsc_on_io_completion(struct hv_device
> *device,
> stor_pkt->vm_srb.data_transfer_length =
> vstor_packet->vm_srb.data_transfer_length;
>
> - storvsc_command_completion(request);
> + storvsc_command_completion(request, stor_device);
>
> if (atomic_dec_and_test(&stor_device->num_outstanding_req) &&
> stor_device->drain_notify)
> @@ -1071,21 +1067,19 @@ static void storvsc_on_io_completion(struct hv_device
> *device,
>
> }
>
> -static void storvsc_on_receive(struct hv_device *device,
> +static void storvsc_on_receive(struct storvsc_device *stor_device,
>struct vstor_packet *vstor_packet,
>struct storvsc_cmd_request *request)
> {
> struct storvsc_scan_work *work;
> - struct storvsc_device *stor_device;
>
> switch (vstor_packet->operation) {
> case VSTOR_OPERATION_COMPLETE_IO:
> - storvsc_on_io_completion(device, vstor_packet, request);
> + storvsc_on_io_completion(stor_device, vstor_packet, request);
> break;
>
> case VSTOR_OPERATION_REMOVE_DEVICE:
> case VSTOR_OPERATION_ENUMERATE_BUS:
> - stor_device = get_in_stor_device(device);
> work = kmalloc(sizeof(struct storvsc_scan_work), GFP_ATOMIC);
> if (!work)
> return;
> @@ -1096,7 +1090,6 @@ static void storvsc_on_receive(struct hv_device *device,
> break;
>
> case VSTOR_OPERATION_FCHBA_DATA:
> - stor_device = get_in_stor_device(device);
> cache_wwn(stor_device, vstor_packet);
> fc_host_node_name(stor_device->host) = stor_device->node_name;
> fc_host_port_name(stor_device->host) = stor_device->port_name;
> @@ -1144,7 +1137,7 @@ static void storvsc_on_channel_callback(void *context)
> vmscsi_size_delta));
> complete(&request->wait_event);
> } else {
> - storvsc_on_receive(device,
> + storvsc_on_receive(stor_device,
> (struct vstor_packet *)packet,
> request);
> }
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Reviewed-by: Johannes Thumshirn
--
Johannes Thumshirn Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284
Re: [PATCH] hisi_sas: use platform_get_irq()
On 10/12/2015 20:08, Rob Herring wrote: On Thu, Dec 10, 2015 at 9:02 AM, John Garry wrote: It is preferred that drivers use platform_get_irq() instead of irq_of_parse_and_map(), so replace. You may be able to stop including of_irq.h with this change. Otherwise, Right, I don't think it is required any longer. I'll generate another patch. Thanks, -- 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/4] scsi: storvsc: Properly support Fibre Channel devices
On Thu, Dec 10, 2015 at 04:14:18PM -0800, K. Y. Srinivasan wrote:
> + ret = vmbus_sendpacket(device->channel, vstor_packet,
> +(sizeof(struct vstor_packet) -
> +vmscsi_size_delta),
> +(unsigned long)request,
> +VM_PKT_DATA_INBAND,
> +VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +
> + if (ret != 0)
> + goto cleanup;
> +
> + t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> + if (t == 0) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
> + }
> +
> + if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> + vstor_packet->status != 0)
> + goto cleanup;
"cleanup" is a misleading name because it doesn't clean up anything.
Do nothing gotos are a pain in the butt and they always introduce bugs.
For example, you appear to have forgotten to set the error code. But
because it's a do-nothing goto it's ambiguous so perhaps returning
success was intended.
Empirically this style of coding causes bugs. It does not prevent them.
It is a bad style if you believe in measuring, evidence and science.
regards,
dan carpenter
--
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/4] scsi: storvsc: Refactor the code in storvsc_channel_init()
On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote:
> @@ -753,27 +740,62 @@ static int storvsc_channel_init(struct hv_device
> *device, bool is_fc)
> VM_PKT_DATA_INBAND,
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> if (ret != 0)
> - goto cleanup;
> + goto done;
>
> t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> if (t == 0) {
> ret = -ETIMEDOUT;
> - goto cleanup;
> + goto done;
> }
>
> + if (!status_check)
> + goto done;
See? This goto looks exactly the same as the earlier buggy goto but
it's actually correct. Meanwhile if you just used an explicit
"return 0;" then it would be easy to understand.
I rant about this all the time but it's because it's bad deliberately.
It's normal to have bugs, but this deliberate stuff really I can't
understand it...
> +
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> vstor_packet->status != 0) {
> ret = -EINVAL;
> - goto cleanup;
> + goto done;
> }
>
> +done:
> + return ret;
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +{
> + struct storvsc_device *stor_device;
> + struct storvsc_cmd_request *request;
> + struct vstor_packet *vstor_packet;
> + int ret, i;
> + int max_chns;
> + bool process_sub_channels = false;
> +
> + stor_device = get_out_stor_device(device);
> + if (!stor_device)
> + return -ENODEV;
> +
> + request = &stor_device->init_request;
> + vstor_packet = &request->vstor_packet;
> +
> + /*
> + * Now, initiate the vsc/vsp initialization protocol on the open
> + * channel
> + */
> + memset(request, 0, sizeof(struct storvsc_cmd_request));
> + vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> + ret = storvsc_execute_vstor_op(device, request, true);
> + if (ret)
> + goto cleanup;
10 lines earlier there is an explicit "return -ENODEV" so it's not as if
writing explicit returns will kill you.
regards,
dan carpenter
--
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] hisi_sas: use platform_get_irq()
It is preferred that drivers use platform_get_irq()
instead of irq_of_parse_and_map(), so replace.
Signed-off-by: John Garry
Acked-by: Rob Herring
diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 30a9ab9..5af2e41 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include
#include
#include
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index 0ed2f92..d543811 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1673,19 +1673,16 @@ static irq_handler_t
fatal_interrupts[HISI_SAS_MAX_QUEUES] = {
static int interrupt_init_v1_hw(struct hisi_hba *hisi_hba)
{
- struct device *dev = &hisi_hba->pdev->dev;
- struct device_node *np = dev->of_node;
+ struct platform_device *pdev = hisi_hba->pdev;
+ struct device *dev = &pdev->dev;
int i, j, irq, rc, idx;
- if (!np)
- return -ENOENT;
-
for (i = 0; i < hisi_hba->n_phy; i++) {
struct hisi_sas_phy *phy = &hisi_hba->phy[i];
idx = i * HISI_SAS_PHY_INT_NR;
for (j = 0; j < HISI_SAS_PHY_INT_NR; j++, idx++) {
- irq = irq_of_parse_and_map(np, idx);
+ irq = platform_get_irq(pdev, idx);
if (!irq) {
dev_err(dev,
"irq init: fail map phy interrupt %d\n",
@@ -1706,7 +1703,7 @@ static int interrupt_init_v1_hw(struct hisi_hba *hisi_hba)
idx = hisi_hba->n_phy * HISI_SAS_PHY_INT_NR;
for (i = 0; i < hisi_hba->queue_count; i++, idx++) {
- irq = irq_of_parse_and_map(np, idx);
+ irq = platform_get_irq(pdev, idx);
if (!irq) {
dev_err(dev, "irq init: could not map cq interrupt
%d\n",
idx);
@@ -1724,7 +1721,7 @@ static int interrupt_init_v1_hw(struct hisi_hba *hisi_hba)
idx = (hisi_hba->n_phy * HISI_SAS_PHY_INT_NR) + hisi_hba->queue_count;
for (i = 0; i < HISI_SAS_FATAL_INT_NR; i++, idx++) {
- irq = irq_of_parse_and_map(np, idx);
+ irq = platform_get_irq(pdev, idx);
if (!irq) {
dev_err(dev, "irq init: could not map fatal interrupt
%d\n",
idx);
--
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 v2] hisi_sas: use platform_get_irq()
On Fri, Dec 11, 2015 at 08:03:21PM +0800, John Garry wrote:
> It is preferred that drivers use platform_get_irq()
> instead of irq_of_parse_and_map(), so replace.
>
> Signed-off-by: John Garry
> Acked-by: Rob Herring
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas.h
> b/drivers/scsi/hisi_sas/hisi_sas.h
> index 30a9ab9..5af2e41 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas.h
> +++ b/drivers/scsi/hisi_sas/hisi_sas.h
> @@ -16,7 +16,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index 0ed2f92..d543811 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> @@ -1673,19 +1673,16 @@ static irq_handler_t
> fatal_interrupts[HISI_SAS_MAX_QUEUES] = {
>
> static int interrupt_init_v1_hw(struct hisi_hba *hisi_hba)
> {
> - struct device *dev = &hisi_hba->pdev->dev;
> - struct device_node *np = dev->of_node;
> + struct platform_device *pdev = hisi_hba->pdev;
> + struct device *dev = &pdev->dev;
> int i, j, irq, rc, idx;
>
> - if (!np)
> - return -ENOENT;
> -
> for (i = 0; i < hisi_hba->n_phy; i++) {
> struct hisi_sas_phy *phy = &hisi_hba->phy[i];
>
> idx = i * HISI_SAS_PHY_INT_NR;
> for (j = 0; j < HISI_SAS_PHY_INT_NR; j++, idx++) {
> - irq = irq_of_parse_and_map(np, idx);
> + irq = platform_get_irq(pdev, idx);
> if (!irq) {
> dev_err(dev,
> "irq init: fail map phy interrupt %d\n",
> @@ -1706,7 +1703,7 @@ static int interrupt_init_v1_hw(struct hisi_hba
> *hisi_hba)
>
> idx = hisi_hba->n_phy * HISI_SAS_PHY_INT_NR;
> for (i = 0; i < hisi_hba->queue_count; i++, idx++) {
> - irq = irq_of_parse_and_map(np, idx);
> + irq = platform_get_irq(pdev, idx);
> if (!irq) {
> dev_err(dev, "irq init: could not map cq interrupt
> %d\n",
> idx);
> @@ -1724,7 +1721,7 @@ static int interrupt_init_v1_hw(struct hisi_hba
> *hisi_hba)
>
> idx = (hisi_hba->n_phy * HISI_SAS_PHY_INT_NR) + hisi_hba->queue_count;
> for (i = 0; i < HISI_SAS_FATAL_INT_NR; i++, idx++) {
> - irq = irq_of_parse_and_map(np, idx);
> + irq = platform_get_irq(pdev, idx);
> if (!irq) {
> dev_err(dev, "irq init: could not map fatal interrupt
> %d\n",
> idx);
> --
> 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
Reviewed-by: Johannes Thumshirn
--
Johannes Thumshirn Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
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
kernel BUG at block/bio.c:1787! while initializing scsi_debug on ppc64 host
Hi, I saw this kernel BUG_ON on 4.4-rc4 kernel, and this can be reproduced easily on ppc64 host by: modprobe scsi_debug sector_size=512 physblk_exp=3 dev_size_mb=256 And I bisected to this commit commit ca369d51b3e1649be4a72addd6d6a168cfb3f537 Author: Martin K. Petersen Date: Fri Nov 13 16:46:48 2015 -0500 block/sd: Fix device-imposed transfer length limits I confirmed by reverting this commit on top of 4.4-rc4 kernel and test passed. Thanks, Eryu P.S. dmesg log [ 817.477557] scsi_debug:sdebug_driver_probe: host protection [ 817.477571] scsi host1: scsi_debug, version 1.85 [20141022], dev_size_mb=256, opts=0x0 [ 817.478202] scsi 1:0:0:0: Direct-Access Linuxscsi_debug 0184 PQ: 0 ANSI: 6 [ 817.478733] sd 1:0:0:0: Attached scsi generic sg1 type 0 [ 817.496144] sd 1:0:0:0: [sdb] 524288 512-byte logical blocks: (268 MB/256 MiB) [ 817.496155] sd 1:0:0:0: [sdb] 4096-byte physical blocks [ 817.506142] sd 1:0:0:0: [sdb] Write Protect is off [ 817.526134] sd 1:0:0:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA [ 817.646163] [ cut here ] [ 817.646168] kernel BUG at block/bio.c:1787! [ 817.646172] Oops: Exception in kernel mode, sig: 5 [#1] [ 817.646174] SMP NR_CPUS=2048 NUMA pSeries [ 817.646178] Modules linked in: scsi_debug(E) nfsv3(E) rpcsec_gss_krb5(E) nfsv4(E) dns_resolver(E) nfs(E) fscache(E) dm_mod(E) loop(E) sg(E) pseries_rng(E) nfsd(E) auth_rpcgss(E) nfs_acl(E) lockd(E) sunrpc(E) grace(E) ip_tables(E) xfs(E) libcrc32c(E) sd_mod(E) ibmvscsi(E) ibmveth(E) scsi_transport_srp(E) [ 817.646205] CPU: 6 PID: 166 Comm: kworker/u321:1 Tainted: GE 4.4.0-rc4 #1 [ 817.646211] Workqueue: events_unbound .async_run_entry_fn [ 817.646215] task: ca0c ti: ca18 task.ti: ca18 [ 817.646218] NIP: c03b1d54 LR: c03c4780 CTR: c03be420 [ 817.646222] REGS: ca1826c0 TRAP: 0700 Tainted: GE (4.4.0-rc4) [ 817.646225] MSR: 800100029032 CR: 24732728 XER: [ 817.646233] CFAR: c03c477c SOFTE: 1 GPR00: c03c4780 ca182940 c1325e00 c0016cebcf00 GPR04: 0240 c0013c5f4d80 0040 GPR08: f0436ac0 0001 GPR12: 24732722 ce743900 f0436ac0 GPR16: c000f9e3eee0 c0010dab 0001 GPR20: 0080 c0016cebcf00 GPR24: c000ff9b5a20 ca182bb8 c0016cebcf88 GPR28: c0016cebcf00 0001 [ 817.646273] NIP [c03b1d54] .bio_split+0x34/0x110 [ 817.646277] LR [c03c4780] .blk_queue_split+0x3b0/0x560 [ 817.646280] Call Trace: [ 817.646282] [ca182940] [ca1829d0] 0xca1829d0 (unreliable) [ 817.646287] [ca1829d0] [c03c4780] .blk_queue_split+0x3b0/0x560 [ 817.646291] [ca182ae0] [c03be460] .blk_queue_bio+0x40/0x430 [ 817.646295] [ca182b80] [c03bc0f0] .generic_make_request+0x150/0x210 [ 817.646299] [ca182c30] [c03bc26c] .submit_bio+0xbc/0x1c0 [ 817.646304] [ca182cf0] [c02cb64c] .submit_bh_wbc+0x19c/0x200 [ 817.646308] [ca182d90] [c02cbb10] .block_read_full_page+0x310/0x410 [ 817.646312] [ca183290] [c02cf11c] .blkdev_readpage+0x1c/0x30 [ 817.646316] [ca183300] [c01e51a0] .do_read_cache_page+0xc0/0x290 [ 817.646321] [ca1833c0] [c03d59f8] .read_dev_sector+0x38/0xb0 [ 817.646325] [ca183440] [c03d977c] .read_lba+0xcc/0x1f0 [ 817.646329] [ca1834f0] [c03da3b8] .efi_partition+0x118/0x780 [ 817.646333] [ca183670] [c03d6fcc] .check_partition+0x14c/0x2e0 [ 817.646337] [ca183700] [c03d6260] .rescan_partitions+0xd0/0x380 [ 817.646341] [ca1837e0] [c02d0b88] .__blkdev_get+0x3d8/0x530 [ 817.646345] [ca1838a0] [c02d0f10] .blkdev_get+0x230/0x4a0 [ 817.646348] [ca1839a0] [c03d3288] .add_disk+0x468/0x4f0 [ 817.646353] [ca183a60] [d2026450] .sd_probe_async+0xf0/0x230 [sd_mod] [ 817.646357] [ca183af0] [c00d23a8] .async_run_entry_fn+0x98/0x200 [ 817.646362] [ca183ba0] [c00c6d74] .process_one_work+0x1a4/0x490 [ 817.646366] [ca183c40] [c00c71dc] .worker_thread+0x17c/0x5a0 [ 817.646369] [ca183d30] [c00ce704] .kthread+0x104/0x130 [ 817.646374] [ca183e30] [c0009534] .ret_from_kernel_thread+0x58/0xa4 [ 817.646377] Instruction dump: [ 817.646379] 3924 7d292378 fba1ffe8 55290ffe fbc1fff0 fb81ffe0 fbe1fff8 7c9e2378 [ 817.646386] 7c7d1b78 f8
Re: [PATCH 07/13] IB: add a proper completion queue abstraction
On Thu, Dec 10, 2015 at 10:42:22AM -0800, Bart Van Assche wrote:
>> +struct ib_cq *ib_alloc_cq(struct ib_device *dev, void *private,
>> +int nr_cqe, int comp_vector, enum ib_poll_context poll_ctx)
>> +{
> > [ ... ]
>> +cq->wc = kmalloc_array(IB_POLL_BATCH, sizeof(*cq->wc), GFP_KERNEL);
>
> Why is the wc array allocated separately instead of being embedded in
> struct ib_cq ? I think the faster completion queues can be created the
> better so if it is possible to eliminate the above kmalloc() call I would
> prefer that.
I originally allocated an embedded aray, but Sagi pointed out that
we'd waste memory for CQs not using the new API, so I changed it.
The embedded one would be quite a bit simpler indeed.
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -457,10 +457,11 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct
>> srp_target_port *target)
>> static void srp_destroy_qp(struct srp_rdma_ch *ch)
>> {
>> static struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
>> -static struct ib_recv_wr wr = { .wr_id = SRP_LAST_WR_ID };
>> +static struct ib_recv_wr wr = { 0 };
>> struct ib_recv_wr *bad_wr;
>> int ret;
>
> Is explicit initialization to "{ 0 }" really needed for static structures ?
It shouldn't be needed, but I can't see how it harms either.
--
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 10/13] IB/srp: use the new CQ API
Hi Bart, thanks for all the reviews. I've updated the git branch with your suggestions and reviewed-by tags. I'm going to wait a little bit longer for other reviews to come in before reposting the 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 2/6] cxlflash: Fix to avoid virtual LUN failover failure
On 12/10/2015 4:53 PM, Uma Krishnan wrote: From: "Matthew R. Ochs" To remedy this scenario, provide feedback back to the application on virtual LUN creation as to which ports the LUN may be accessed. LUN's spanning both ports are candidates for a retry in a presence of an I/O failure. Signed-off-by: Matthew R. Ochs Acked-by: Manoj Kumar -- 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: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management
On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote: > On 11/17/2015 03:20 PM, Martin K. Petersen wrote: > >> "Lee" == Lee Duncan writes: > > > > Lee> Martin: I will be glad to update the patch, creating a modprobe > > Lee> parameter as suggested, if you find this acceptable. > > > > For development use a module parameter would be fine. But I am concerned > > about our support folks that rely on the incrementing host number when > > analyzing customer log files. > > > > Ewan: How do you folks feel about this change? > > > > Ewan? Personally, I think having host numbers that increase essentially without limit (I think I've seen this with iSCSI sessions) are a problem, the numbers start to lose meaning for people when they are not easily recognizable. Yes, it can help when you're analyzing a log file, but it seems to me that you would want to track the host state throughout anyway, so you could just follow the number as it changes. If we change the behavior, we have to change documentation, and our support people will get calls. But that's not a reason not to do it. -Ewan -- 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] ses: fix additional element traversal bug
KASAN found that our additional element processing scripts drop off
the end of the VPD page into unallocated space. The reason is that
not every element has additional information but our traversal
routines think they do, leading to them expecting far more additional
information than is present. Fix this by adding a gate to the
traversal routine so that it only processes elements that are expected
to have additional information (list is in SES-2 section 6.1.13.1:
Additional Element Status diagnostic page overview)
Reported-by: Pavel Tikhomirov
Tested-by: Pavel Tikhomirov
Cc: sta...@vger.kernel.org
Signed-off-by: James Bottomley
---
diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 1736935..53ef1cb 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -561,7 +561,15 @@ static void ses_enclosure_data_process(struct
enclosure_device *edev,
if (desc_ptr)
desc_ptr += len;
- if (addl_desc_ptr)
+ if (addl_desc_ptr &&
+ /* only find additional descriptions for specific
devices */
+ (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
+type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE ||
+type_ptr[0] == ENCLOSURE_COMPONENT_SAS_EXPANDER ||
+/* these elements are optional */
+type_ptr[0] ==
ENCLOSURE_COMPONENT_SCSI_TARGET_PORT ||
+type_ptr[0] ==
ENCLOSURE_COMPONENT_SCSI_INITIATOR_PORT ||
+type_ptr[0] ==
ENCLOSURE_COMPONENT_CONTROLLER_ELECTRONICS))
addl_desc_ptr += addl_desc_ptr[1] + 2;
}
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index 7be22da..a4cf57c 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -29,7 +29,11 @@
/* A few generic types ... taken from ses-2 */
enum enclosure_component_type {
ENCLOSURE_COMPONENT_DEVICE = 0x01,
+ ENCLOSURE_COMPONENT_CONTROLLER_ELECTRONICS = 0x07,
+ ENCLOSURE_COMPONENT_SCSI_TARGET_PORT = 0x14,
+ ENCLOSURE_COMPONENT_SCSI_INITIATOR_PORT = 0x15,
ENCLOSURE_COMPONENT_ARRAY_DEVICE = 0x17,
+ ENCLOSURE_COMPONENT_SAS_EXPANDER = 0x18,
};
/* ses-2 common element status */
--
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 10/13] IB/srp: use the new CQ API
On 12/11/2015 09:22 AM, Christoph Hellwig wrote: > Hi Bart, > > thanks for all the reviews. I've updated the git branch with your > suggestions and reviewed-by tags. I'm going to wait a little bit > longer for other reviews to come in before reposting the series. Indeed, thanks for all the catches Bart. This patchset, with Bart's fixups, looks good to me. -- Doug Ledford GPG KeyID: 0E572FDD signature.asc Description: OpenPGP digital signature
Re: [dm-devel] [PATCH 0/15] copy offload patches
On Thu, Dec 10, 2015 at 11:06:04PM -0600, Mike Christie wrote: > > 2. Start REQ_OP_READ off at non-zero to try and shake out code that was > not converted. > > There are a several places where we assume reads are zero and writes are > 1 for things like indexing in arrays (like blktrace's ddir_act or dm > starts), passing into block functions (like nvme_alloc_request's call of > blk_mq_alloc_request), and if/else's. I am not done fixing all of them > and testing. If this turns out to be too painful we should just skip it. I though it would be a nie debugging aid, but we don't need to do it at any cost. -- 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/4] scsi: storvsc: Refactor the code in storvsc_channel_init()
> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Friday, December 11, 2015 2:41 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH 3/4] scsi: storvsc: Refactor the code in
> storvsc_channel_init()
>
> On Thu, Dec 10, 2015 at 04:14:19PM -0800, K. Y. Srinivasan wrote:
> > @@ -753,27 +740,62 @@ static int storvsc_channel_init(struct hv_device
> *device, bool is_fc)
> >VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > if (ret != 0)
> > - goto cleanup;
> > + goto done;
> >
> > t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > if (t == 0) {
> > ret = -ETIMEDOUT;
> > - goto cleanup;
> > + goto done;
> > }
> >
> > + if (!status_check)
> > + goto done;
>
> See? This goto looks exactly the same as the earlier buggy goto but
> it's actually correct. Meanwhile if you just used an explicit
> "return 0;" then it would be easy to understand.
>
> I rant about this all the time but it's because it's bad deliberately.
> It's normal to have bugs, but this deliberate stuff really I can't
> understand it...
>
> > +
> > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > vstor_packet->status != 0) {
> > ret = -EINVAL;
> > - goto cleanup;
> > + goto done;
> > }
> >
> > +done:
> > + return ret;
> > +}
> > +
> > +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> > +{
> > + struct storvsc_device *stor_device;
> > + struct storvsc_cmd_request *request;
> > + struct vstor_packet *vstor_packet;
> > + int ret, i;
> > + int max_chns;
> > + bool process_sub_channels = false;
> > +
> > + stor_device = get_out_stor_device(device);
> > + if (!stor_device)
> > + return -ENODEV;
> > +
> > + request = &stor_device->init_request;
> > + vstor_packet = &request->vstor_packet;
> > +
> > + /*
> > +* Now, initiate the vsc/vsp initialization protocol on the open
> > +* channel
> > +*/
> > + memset(request, 0, sizeof(struct storvsc_cmd_request));
> > + vstor_packet->operation =
> VSTOR_OPERATION_BEGIN_INITIALIZATION;
> > + ret = storvsc_execute_vstor_op(device, request, true);
> > + if (ret)
> > + goto cleanup;
>
> 10 lines earlier there is an explicit "return -ENODEV" so it's not as if
> writing explicit returns will kill you.
Thanks Dan; I will cleanup the code and resend.
Regards,
K. Y
--
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/4] scsi: storvsc: Properly support Fibre Channel devices
> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Friday, December 11, 2015 2:25 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH 2/4] scsi: storvsc: Properly support Fibre Channel devices
>
> On Thu, Dec 10, 2015 at 04:14:18PM -0800, K. Y. Srinivasan wrote:
> > + ret = vmbus_sendpacket(device->channel, vstor_packet,
> > + (sizeof(struct vstor_packet) -
> > + vmscsi_size_delta),
> > + (unsigned long)request,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +
> > + if (ret != 0)
> > + goto cleanup;
> > +
> > + t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > + if (t == 0) {
> > + ret = -ETIMEDOUT;
> > + goto cleanup;
> > + }
> > +
> > + if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > + vstor_packet->status != 0)
> > + goto cleanup;
>
> "cleanup" is a misleading name because it doesn't clean up anything.
> Do nothing gotos are a pain in the butt and they always introduce bugs.
> For example, you appear to have forgotten to set the error code. But
> because it's a do-nothing goto it's ambiguous so perhaps returning
> success was intended.
>
> Empirically this style of coding causes bugs. It does not prevent them.
> It is a bad style if you believe in measuring, evidence and science.
Thanks Dan. I will fix this and resend.
K. Y
--
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/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet
> -Original Message-
> From: Johannes Thumshirn [mailto:jthumsh...@suse.de]
> Sent: Friday, December 11, 2015 12:48 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> de...@linuxdriverproject.org; oher...@suse.com;
> jbottom...@parallels.com; h...@infradead.org; linux-scsi@vger.kernel.org;
> a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> martin.peter...@oracle.com
> Subject: Re: [PATCH 1/4] scsi: storvsc: Fix a bug in the layout of the
> hv_fc_wwn_packet
>
> On Thu, Dec 10, 2015 at 04:14:17PM -0800, K. Y. Srinivasan wrote:
> > The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in
> Linux match
> > the Window's definition.
> >
> > Signed-off-by: K. Y. Srinivasan
> > Reviewed-by: Long Li
> > Tested-by: Alex Ng
> > ---
> > drivers/scsi/storvsc_drv.c |5 ++---
> > 1 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index c41f674..00bb4bd 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -92,9 +92,8 @@ enum vstor_packet_operation {
> > */
> >
> > struct hv_fc_wwn_packet {
> > - boolprimary_active;
> > - u8 reserved1;
> > - u8 reserved2;
> > + u8 primary_active;
> > + u8 reserved1[3];
> > u8 primary_port_wwn[8];
> > u8 primary_node_wwn[8];
> > u8 secondary_port_wwn[8];
> > --
> > 1.7.4.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
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.ker
> nel.org%2fmajordomo-
> info.html&data=01%7c01%7ckys%40microsoft.com%7cb6c3be0d1b474ac1374
> 908d30207c170%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=7hfMcn
> qW32KiJgr7JkXsC4P0CnnQvAearwu7LzyWaqM%3d
>
> Should this go through stable as well?
> If yes:
> Fixes: 8b612fa23 '[SCSI] storvsc: Update the storage protocol to win8 level'
> Cc: sta...@vger.kernel.org # v3.11+
This is the first time we are using this data structure and so I don't think we
need
to port it back to stable.
Regards,
K. Y
> Reviewed-by: Johannes Thumshirn
>
> --
> Johannes Thumshirn Storage
> jthumsh...@suse.de+49 911 74053 689
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
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
