RE: [PATCH v3] uas: replace WARN_ON_ONCE() with lockdep_assert_held()
-Original Message- From: gre...@linuxfoundation.org [mailto:gre...@linuxfoundation.org] Sent: Tuesday, August 19, 2014 3:01 PM To: Sharma, Sanjeev Cc: kra...@redhat.com; mdharm-...@one-eyed-alien.net; linux-...@vger.kernel.org; linux-ker...@vger.kernel.org; linux-scsi@vger.kernel.org; Hans de Goede Subject: Re: [PATCH v3] uas: replace WARN_ON_ONCE() with lockdep_assert_held() On Tue, Aug 19, 2014 at 06:33:04AM +, Sharma, Sanjeev wrote: > Hi Greg, > > Any feedback on this patch ? The merge window ended 2 days ago, _and_ I'm at the kernel summit this week, _and_ my queue is currently sitting at: $ mdfrm -c ~/mail/todo/ 1317 messages in /home/gregkh/mail/todo/ So please be patient. I'll get to it in a few weeks. Please let me know when this is going to be merged ? Thanks Sanjeev greg k-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 1/5] SES: close potential registration race
On 08/25/2014 07:34 PM, Song Liu wrote: > From: Song Liu [mailto:songliubrav...@fb.com] > Sent: Monday, August 25, 2014 10:26 AM > To: Song Liu > Cc: Dan Williams; Hannes Reinecke > Subject: [PATCH 1/5] SES: close potential registration race > > From: Dan Williams > > The slot and address fields have a small window of instability when userspace > can read them before initialization. Separate enclosure_component allocation > from registration. > > Signed-off-by: Dan Williams > Signed-off-by: Song Liu > Reviewed-by: Jens Axboe > Cc: Hannes Reinecke Please run the patch against checkpatch.pl. It does have quite some coding-style issues. Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] SES: add enclosure logical id
On 08/25/2014 07:34 PM, Song Liu wrote:
> From: Song Liu [mailto:songliubrav...@fb.com]
> Sent: Monday, August 25, 2014 10:26 AM
> To: Song Liu
> Cc: Dan Williams; Hannes Reinecke
> Subject: [PATCH 3/5] SES: add enclosure logical id
>
> From: Dan Williams
>
> Export the NAA logical id for the enclosure. This is optionally available
> from the sas_transport_class, but it is really a property of the enclosure.
>
> Signed-off-by: Dan Williams
> Signed-off-by: Song Liu
> Reviewed-by: Jens Axboe
> Cc: Hannes Reinecke
> ---
> drivers/misc/enclosure.c | 13 +
> drivers/scsi/ses.c| 9 +
> include/linux/enclosure.h | 1 +
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index
> 15faf61..646068a 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -395,8 +395,21 @@ static ssize_t components_show(struct device *cdev, }
> static DEVICE_ATTR_RO(components);
>
> +static ssize_t id_show(struct device *cdev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct enclosure_device *edev = to_enclosure_device(cdev);
> +
> + if (edev->cb->show_id)
> + return edev->cb->show_id(edev, buf);
> + return 0;
> +}
> +static DEVICE_ATTR_RO(id);
> +
> static struct attribute *enclosure_class_attrs[] = {
> &dev_attr_components.attr,
> + &dev_attr_id.attr,
> NULL,
> };
> ATTRIBUTE_GROUPS(enclosure_class);
Maybe you should return -EINVAL or something here; '0' would mean an
enclosure id of length '0', which is a different meaning from
'enclosure id not available'.
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 8f0a62a..61deb4e
> 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -258,6 +258,14 @@ static int ses_set_active(struct enclosure_device *edev,
> return ses_set_page2_descriptor(edev, ecomp, desc); }
>
> +static int ses_show_id(struct enclosure_device *edev, char *buf) {
> + struct ses_device *ses_dev = edev->scratch;
> + unsigned long long id = get_unaligned_be64(ses_dev->page1+8+4);
> +
> + return sprintf(buf, "%#llx\n", id);
> +}
> +
> static struct enclosure_component_callbacks ses_enclosure_callbacks = {
> .get_fault = ses_get_fault,
> .set_fault = ses_set_fault,
> @@ -265,6 +273,7 @@ static struct enclosure_component_callbacks
> ses_enclosure_callbacks = {
> .get_locate = ses_get_locate,
> .set_locate = ses_set_locate,
> .set_active = ses_set_active,
> + .show_id= ses_show_id,
> };
>
> struct ses_host_edev {
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h index
> a835d33..807622b 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -79,6 +79,7 @@ struct enclosure_component_callbacks {
> int (*set_locate)(struct enclosure_device *,
> struct enclosure_component *,
> enum enclosure_component_setting);
> + int (*show_id)(struct enclosure_device *, char *buf);
> };
>
>
> --
> 1.8.1
>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 2/5] SES: generate KOBJ_CHANGE on enclosure attach
On 08/25/2014 07:34 PM, Song Liu wrote: > From: Song Liu [mailto:songliubrav...@fb.com] > Sent: Monday, August 25, 2014 10:26 AM > To: Song Liu > Cc: Dan Williams; Hannes Reinecke > Subject: [PATCH 2/5] SES: generate KOBJ_CHANGE on enclosure attach > > From: Dan Williams > > In support of a /dev/disk/by-slot populated with data from the enclosure and > ses modules udev needs notification when the new interface files/links are > available. Otherwise, any udev rules specified for the disk cannot assume > that the enclosure topology has settled. > > Signed-off-by: Dan Williams > Signed-off-by: Song Liu > Reviewed-by: Jens Axboe > Cc: Hannes Reinecke > --- > drivers/scsi/ses.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index c2e8a98..8f0a62a > 100644 > --- a/drivers/scsi/ses.c > +++ b/drivers/scsi/ses.c > @@ -349,7 +349,8 @@ static int ses_enclosure_find_by_addr(struct > enclosure_device *edev, > if (scomp->addr != efd->addr) > continue; > > - enclosure_add_device(edev, i, efd->dev); > + if (enclosure_add_device(edev, i, efd->dev) == 0) > + kobject_uevent(&efd->dev->kobj, KOBJ_CHANGE); > return 1; > } > return 0; > -- > 1.8.1 > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 4/5] SES: add reliable slot attribute
On 08/25/2014 07:34 PM, Song Liu wrote:
> From: Song Liu [mailto:songliubrav...@fb.com]
> Sent: Monday, August 25, 2014 10:26 AM
> To: Song Liu
> Cc: Dan Williams; Hannes Reinecke
> Subject: [PATCH 4/5] SES: add reliable slot attribute
>
> From: Dan Williams
>
> The name provided by firmware is in a vendor specific format, publish the
> slot number to have a reliable mechanism for identifying slots across
> firmware implementations. If the enclosure does not provide a slot number
> fallback to the component number which is guaranteed unique, and usually
> mirrors the slot number.
>
> Cleaned up the unused ses_component.desc in the process.
>
> Signed-off-by: Dan Williams
> Signed-off-by: Song Liu
> Reviewed-by: Jens Axboe
> Cc: Hannes Reinecke
> ---
> drivers/misc/enclosure.c | 20 +++-
> drivers/scsi/ses.c| 17 -
> include/linux/enclosure.h | 1 +
> 3 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index
> 646068a..de335bc 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -145,8 +145,10 @@ enclosure_register(struct device *dev, const char *name,
> int components,
> if (err)
> goto err;
>
> - for (i = 0; i < components; i++)
> + for (i = 0; i < components; i++) {
> edev->component[i].number = -1;
> + edev->component[i].slot = -1;
> + }
>
> mutex_lock(&container_list_lock);
> list_add_tail(&edev->node, &container_list); @@ -552,6 +554,20 @@
> static ssize_t get_component_type(struct device *cdev,
> return snprintf(buf, 40, "%s\n", enclosure_type[ecomp->type]); }
>
> +static ssize_t get_component_slot(struct device *cdev,
> + struct device_attribute *attr, char *buf) {
> + struct enclosure_component *ecomp = to_enclosure_component(cdev);
> + int slot;
> +
> + /* if the enclosure does not override then use 'number' as a stand-in */
> + if (ecomp->slot >= 0)
> + slot = ecomp->slot;
> + else
> + slot = ecomp->number;
> +
> + return snprintf(buf, 40, "%d\n", slot); }
>
> static DEVICE_ATTR(fault, S_IRUGO | S_IWUSR, get_component_fault,
> set_component_fault);
> @@ -562,6 +578,7 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR,
> get_component_active, static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR,
> get_component_locate,
> set_component_locate);
> static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL);
> +static DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
>
> static struct attribute *enclosure_component_attrs[] = {
> &dev_attr_fault.attr,
> @@ -569,6 +586,7 @@ static struct attribute *enclosure_component_attrs[] = {
> &dev_attr_active.attr,
> &dev_attr_locate.attr,
> &dev_attr_type.attr,
> + &dev_attr_slot.attr,
> NULL
> };
> ATTRIBUTE_GROUPS(enclosure_component);
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 61deb4e..bafa301
> 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -47,7 +47,6 @@ struct ses_device {
>
> struct ses_component {
> u64 addr;
> - unsigned char *desc;
> };
>
> static int ses_probe(struct device *dev) @@ -307,19 +306,26 @@ static void
> ses_process_descriptor(struct enclosure_component *ecomp,
> int invalid = desc[0] & 0x80;
> enum scsi_protocol proto = desc[0] & 0x0f;
> u64 addr = 0;
> + int slot = -1;
> struct ses_component *scomp = ecomp->scratch;
> unsigned char *d;
>
> - scomp->desc = desc;
> -
> if (invalid)
> return;
>
> switch (proto) {
> + case SCSI_PROTOCOL_FCP:
> + if (eip) {
> + d = desc + 4;
> + slot = d[3];
> + }
> + break;
> case SCSI_PROTOCOL_SAS:
> - if (eip)
> + if (eip) {
> + d = desc + 4;
> + slot = d[3];
> d = desc + 8;
> - else
> + } else
> d = desc + 4;
> /* only take the phy0 addr */
> addr = (u64)d[12] << 56 |
> @@ -335,6 +341,7 @@ static void ses_process_descriptor(struct
> enclosure_component *ecomp,
> /* FIXME: Need to add more protocols than just SAS */
> break;
> }
> + ecomp->slot = slot;
> scomp->addr = addr;
> }
>
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h index
> 807622b..0f826c1 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -92,6 +92,7 @@ struct enclosure_component {
> int fault;
> int active;
> int locate;
> + int slot;
> enum enclosure_status status;
> };
>
> --
> 1.8.1
>
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes Reinecke
Re: [PATCH 5/5] SES: Add power_status to SES enclosure component
On 08/25/2014 07:34 PM, Song Liu wrote:
> From: Song Liu [mailto:songliubrav...@fb.com]
> Sent: Monday, August 25, 2014 10:26 AM
> To: Song Liu
> Cc: Hannes Reinecke
> Subject: [PATCH 5/5] SES: Add power_status to SES enclosure component
>
> Add power_status to SES enclosure component, so we can power on/off the HDDs
> behind the enclosure.
>
> Check firmware status in ses_set_* before sending control pages to firmware.
>
> Signed-off-by: Song Liu
> Acked-by: Dan Williams
> Reviewed-by: Jens Axboe
> Cc: Hannes Reinecke
> ---
> drivers/misc/enclosure.c | 29 ++
> drivers/scsi/ses.c| 98
> ++-
> include/linux/enclosure.h | 6 +++
> 3 files changed, 124 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c index
> de335bc..0331dfe 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -148,6 +148,7 @@ enclosure_register(struct device *dev, const char *name,
> int components,
> for (i = 0; i < components; i++) {
> edev->component[i].number = -1;
> edev->component[i].slot = -1;
> + edev->component[i].power_status = 1;
> }
>
> mutex_lock(&container_list_lock);
> @@ -546,6 +547,31 @@ static ssize_t set_component_locate(struct device *cdev,
> return count;
> }
>
> +static ssize_t get_component_power_status(struct device *cdev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> + struct enclosure_component *ecomp = to_enclosure_component(cdev);
> +
> + if (edev->cb->get_power_status)
> + edev->cb->get_power_status(edev, ecomp);
> + return snprintf(buf, 40, "%d\n", ecomp->power_status); }
> +
> +static ssize_t set_component_power_status(struct device *cdev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct enclosure_device *edev = to_enclosure_device(cdev->parent);
> + struct enclosure_component *ecomp = to_enclosure_component(cdev);
> + int val = simple_strtoul(buf, NULL, 0);
> +
> + if (edev->cb->set_power_status)
> + edev->cb->set_power_status(edev, ecomp, val);
> + return count;
> +}
> +
Just using a number here doesn't seem to be very instructive; what
is this number? Can't we have a decode setting here to allow even
the uninitiated to do some sensible decisions here?
> static ssize_t get_component_type(struct device *cdev,
> struct device_attribute *attr, char *buf) {
> @@ -577,6 +603,8 @@ static DEVICE_ATTR(active, S_IRUGO | S_IWUSR,
> get_component_active,
> set_component_active);
> static DEVICE_ATTR(locate, S_IRUGO | S_IWUSR, get_component_locate,
> set_component_locate);
> +static DEVICE_ATTR(power_status, S_IRUGO | S_IWUSR,
> get_component_power_status,
> +set_component_power_status);
> static DEVICE_ATTR(type, S_IRUGO, get_component_type, NULL); static
> DEVICE_ATTR(slot, S_IRUGO, get_component_slot, NULL);
>
> @@ -585,6 +613,7 @@ static struct attribute *enclosure_component_attrs[] = {
> &dev_attr_status.attr,
> &dev_attr_active.attr,
> &dev_attr_locate.attr,
> + &dev_attr_power_status.attr,
> &dev_attr_type.attr,
> &dev_attr_slot.attr,
> NULL
> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index bafa301..ea6b262
> 100644
> --- a/drivers/scsi/ses.c
> +++ b/drivers/scsi/ses.c
> @@ -67,6 +67,20 @@ static int ses_probe(struct device *dev) #define
> SES_TIMEOUT (30 * HZ) #define SES_RETRIES 3
>
> +static void init_device_slot_control(unsigned char *dest_desc,
> + struct enclosure_component *ecomp,
> + unsigned char *status)
> +{
> + memcpy(dest_desc, status, 4);
> + dest_desc[0] = 0;
> + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */
> + if (ecomp->type == ENCLOSURE_COMPONENT_DEVICE)
> + dest_desc[1] = 0;
> + dest_desc[2] &= 0xde;
> + dest_desc[3] &= 0x3c;
> +}
> +
> +
> static int ses_recv_diag(struct scsi_device *sdev, int page_code,
>void *buf, int bufflen)
> {
> @@ -178,14 +192,22 @@ static int ses_set_fault(struct enclosure_device *edev,
> struct enclosure_component *ecomp,
>enum enclosure_component_setting val) {
> - unsigned char desc[4] = {0 };
> + unsigned char desc[4];
> + unsigned char *desc_ptr;
> +
> + desc_ptr = ses_get_page2_descriptor(edev, ecomp);
> +
> + if (!desc_ptr)
> + return -EIO;
> +
> + init_device_slot_control(desc, ecomp, desc_ptr);
>
> switch (val) {
> case ENCLOSURE_SETT
Request for comment: monitoring Fibre ports bandwidth usage
Hi guys,
I wrote the script below because I wanted to monitor the bandwidth
usage of my fibre port connected to the SAN. I had some difficulties
because that information is not easy to grab: drivers don't implement
all stats files in sysfs. Some populate fcp_input_megabytes and
fcp_output_megabytes, others populate only rx_words and tx_words.
My main question is : what's the length of a word in the fibre
langague? I found it is 32 bits from RFC 6307
(http://tools.ietf.org/html/rfc6307#section-3.3.1). Is it correct?
Here is the script:
#! /bin/bash
[ ! -d /sys/class/fc_host ] && exit 0
cd /sys/class/fc_host
function get_fc_usage_by_mb {
[ ! -e /var/tmp/fc_stats_$5.mb ] && echo "$4 $1 $2" >
/var/tmp/fc_stats_$5.mb && echo Creating history file && continue
old_stats=$(cat /var/tmp/fc_stats_$5.mb)
before=$(echo $old_stats | awk '{print $1}')
old_rx_mb=$(echo $old_stats | awk '{print $2}')
old_tx_mb=$(echo $old_stats | awk '{print $3}')
echo "$4 $1 $2" > /var/tmp/fc_stats_$5.mb
# In the math below, why dividing by 80?
# To get percentages, we got the maximum speed of the port in
gigabits. So we have to divide by 8 to have Gigabytes
# But the system info are in megabytes so we have to divide by 1000
# And finaly, we have to multiply by 100 to get the percentage
perl -se 'printf "%.2f", (($new_rx_mb - $old_rx_mb) + ($new_tx_mb
- $old_tx_mb)) / ($now - $before) / $speed /80' -- -new_rx_mb=$1
-new_tx_mb=$2 -speed=$3 -now=$4 -before=$before -old_rx_mb=$old_rx_mb
-old_tx_mb=$old_tx_mb
}
function get_fc_usage_by_w {
[ ! -e /var/tmp/fc_stats_$5.w ] && echo "$4 $1 $2" >
/var/tmp/fc_stats_$5.w && echo Creating history file && continue
old_stats=$(cat /var/tmp/fc_stats_$5.w)
before=$(echo $old_stats | awk '{print $1}')
old_rx_w=$(echo $old_stats | awk '{print $2}')
old_tx_w=$(echo $old_stats | awk '{print $3}')
echo "$4 $1 $2" > /var/tmp/fc_stats_$5.w
perl -se 'printf "%.2f", (($new_rx_w - $old_rx_w) + ($new_tx_w -
$old_tx_w)) * 32 / ($now - $before) *100 / $speed' -- -new_rx_w=$1
-new_tx_w=$2 -speed=$3 -now=$4 -before=$before -old_rx_w=$old_rx_w
-old_tx_w=$old_tx_w
}
for i in * ; do
speed_str=$(cat $i/speed | sed -r 's/^.*([0-9]+) *[Gg]b.*$/\1/')
if [ -z "$speed_str" ]; then
echo "Unable to understand speed: \"$(cat $i/speed)\""
exit 0
fi
# Multiply with string method (concatenation)
speed="${speed_str}0"
wwn=$(cat $i/port_name)
now=$(date +%s)
rx_mb=$(($(cat $i/statistics/fcp_input_megabytes )))
tx_mb=$(($(cat $i/statistics/fcp_output_megabytes)))
rx_w=$(($(cat $i/statistics/rx_words)))
tx_w=$(($(cat $i/statistics/tx_words)))
if [ $(($rx_mb + $tx_mb)) -gt 0 ]; then
val=$(get_fc_usage_by_mb $rx_mb $tx_mb $speed $now $i)
elif [ $(($rx_w + $tx_w )) -gt 0 ]; then
val=$(get_fc_usage_by_w $rx_w $tx_w $speed $now $i)
fi
[ -n "$val" ] && echo "$wwn;$val;%"
done
--
Nicolas MICHEL
--
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: [SCSI] be2iscsi: adding functionality to change network settings using iscsiadm
Hello Mike Christie,
The patch 0e43895ec1f4: "[SCSI] be2iscsi: adding functionality to
change network settings using iscsiadm" from Apr 3, 2012, leads to
the following static checker warning:
drivers/scsi/be2iscsi/be_mgmt.c:945 mgmt_static_ip_modify()
error: 'ip_param->len' from user is not capped properly
drivers/scsi/be2iscsi/be_mgmt.c
940 req->ip_params.ip_record.ip_addr.size_of_structure =
941 sizeof(struct be_ip_addr_subnet_format);
942 req->ip_params.ip_record.ip_addr.ip_type = ip_type;
943
944 if (ip_action == IP_ACTION_ADD) {
945 memcpy(req->ip_params.ip_record.ip_addr.addr,
ip_param->value,
946 ip_param->len);
947
948 if (subnet_param)
949
memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
950 subnet_param->value, subnet_param->len);
^
951 } else {
952 memcpy(req->ip_params.ip_record.ip_addr.addr,
953 if_info->ip_addr.addr, ip_param->len);
^
954
955 memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
956 if_info->ip_addr.subnet_mask, ip_param->len);
957 }
These memcpy()s can overflow. It seems root only but it makes the
static checker complain.
One call tree is:
beiscsi_set_static_ip() <- gets iface_ip.
-> mgmt_set_ip()
-> mgmt_static_ip_modify()
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 v3 11/13] lpfc: Use pci_enable_msix_range() instead of pci_enable_msix()
On Mon, Aug 18, 2014 at 08:01:51AM +0200, Alexander Gordeev wrote: > As result of deprecation of MSI-X/MSI enablement functions > pci_enable_msix() and pci_enable_msi_block() all drivers > using these two interfaces need to be updated to use the > new pci_enable_msi_range() or pci_enable_msi_exact() > and pci_enable_msix_range() or pci_enable_msix_exact() > interfaces. James? -- Regards, Alexander Gordeev agord...@redhat.com -- 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 11/13] lpfc: Use pci_enable_msix_range() instead of pci_enable_msix()
On Thu, Sep 04, 2014 at 02:29:39PM +0100, Alexander Gordeev wrote: > On Mon, Aug 18, 2014 at 08:01:51AM +0200, Alexander Gordeev wrote: > > As result of deprecation of MSI-X/MSI enablement functions > > pci_enable_msix() and pci_enable_msi_block() all drivers > > using these two interfaces need to be updated to use the > > new pci_enable_msi_range() or pci_enable_msi_exact() > > and pci_enable_msix_range() or pci_enable_msix_exact() > > interfaces. > > James? It's part of the lpfc update he recently sent out. It's high up on my list of things to review. -- 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: Problem with USB-to-SATA adapters (was: AS2105-based enclosure size issues with >2TB HDDs)
> From: James Bottomley > Before we embark on elaborate hacks, why don't we just make the capacity > writeable (by root) in sysfs from userspace (will require block change)? > We can then encode all the nasty heuristics (including gpt reading) in > userspace as a udev rule. Looking in from the outside, this makes sense to me. All the nasty hueristics can be put in userspace -- perhaps as even a special-purpose program, where we can directly warn the user as to what he's getting himself into. (I do not demand that this all work automatically.) And the hueristics can be improved easily, without kernel changes. The only gotcha that I see is that once the recorded device size is changed, the kernel may now consider the partition table to be valid, and should now parse it and set up the special device numbers for the partitions. So that needs to get triggered properly. I suppose there is some complexity if the block-handling layer already has blocks cached and the device size is reduced below the addresses of the cached blocks. But as long as the kernel doesn't crash or write blocks in incorrect places, I don't see that as a problem -- if you set the device size as less than the block numbers you've already written to, that's your problem. Dale -- 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 08/20] scsi: use 'bool' as return value for scsi_normalize_sense()
(2014/09/03 19:06), Hannes Reinecke wrote:
> Convert scsi_normalize_sense() and frieds to return 'bool'
> instead of an integer.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/scsi_error.c | 14 +++---
> drivers/scsi/scsi_lib.c | 2 +-
> include/scsi/scsi_eh.h| 12 ++--
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 6c99624..8a6d382 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -2408,20 +2408,20 @@ EXPORT_SYMBOL(scsi_reset_provider);
>* responded to a SCSI command with the CHECK_CONDITION status.
>*
>* Return value:
> - * 1 if valid sense data information found, else 0;
> + * true if valid sense data information found, else false;
>*/
> -int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> - struct scsi_sense_hdr *sshdr)
> +bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> + struct scsi_sense_hdr *sshdr)
> {
> if (!sense_buffer || !sb_len)
> - return 0;
> + return false;
>
> memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
>
> sshdr->response_code = (sense_buffer[0] & 0x7f);
>
> if (!scsi_sense_valid(sshdr))
> - return 0;
> + return false;
>
> if (sshdr->response_code >= 0x72) {
> /*
> @@ -2451,11 +2451,11 @@ int scsi_normalize_sense(const u8 *sense_buffer, int
> sb_len,
> }
> }
>
> - return 1;
> + return true;
> }
> EXPORT_SYMBOL(scsi_normalize_sense);
>
> -int scsi_command_normalize_sense(struct scsi_cmnd *cmd,
> +bool scsi_command_normalize_sense(struct scsi_cmnd *cmd,
>struct scsi_sense_hdr *sshdr)
> {
> return scsi_normalize_sense(cmd->sense_buffer,
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index de178f7..8cad004 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -830,7 +830,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
> int good_bytes)
> struct request *req = cmd->request;
> int error = 0;
> struct scsi_sense_hdr sshdr;
> - int sense_valid = 0;
> + bool sense_valid = false;
> int sense_deferred = 0;
> enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
> ACTION_DELAYED_RETRY} action;
> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
> index 06a8790..1427a66 100644
> --- a/include/scsi/scsi_eh.h
> +++ b/include/scsi/scsi_eh.h
> @@ -27,7 +27,7 @@ struct scsi_sense_hdr { /* See SPC-3 section
> 4.5 */
> u8 additional_length; /* always 0 for fixed sense format */
> };
>
> -static inline int scsi_sense_valid(struct scsi_sense_hdr *sshdr)
> +static inline bool scsi_sense_valid(struct scsi_sense_hdr *sshdr)
> {
> if (!sshdr)
> return 0;
Should be "return false;"
The others in this patch look good.
Thanks,
Yoshihiro YUNOMAE
> @@ -42,12 +42,12 @@ extern void scsi_eh_flush_done_q(struct list_head
> *done_q);
> extern void scsi_report_bus_reset(struct Scsi_Host *, int);
> extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
> extern int scsi_block_when_processing_errors(struct scsi_device *);
> -extern int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> - struct scsi_sense_hdr *sshdr);
> -extern int scsi_command_normalize_sense(struct scsi_cmnd *cmd,
> - struct scsi_sense_hdr *sshdr);
> +extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
> + struct scsi_sense_hdr *sshdr);
> +extern bool scsi_command_normalize_sense(struct scsi_cmnd *cmd,
> + struct scsi_sense_hdr *sshdr);
>
> -static inline int scsi_sense_is_deferred(struct scsi_sense_hdr *sshdr)
> +static inline bool scsi_sense_is_deferred(struct scsi_sense_hdr *sshdr)
> {
> return ((sshdr->response_code >= 0x70) && (sshdr->response_code & 1));
> }
>
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae...@hitachi.com
--
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 12/20] scsi: merge print_opcode_name()
(2014/09/03 19:06), Hannes Reinecke wrote:
> Instead of having two versions of print_opcode_name() we
> should be consolidating them into one version.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/constants.c | 90
> +++-
> 1 file changed, 35 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 364349c..3aee43b 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -30,6 +30,11 @@
> #define THIRD_PARTY_COPY_IN 0x84
>
>
> +struct sa_name_list {
> + int cmd;
> + const struct value_name_pair *arr;
> + int arr_sz;
> +};
>
> #ifdef CONFIG_SCSI_CONSTANTS
> static const char * cdb_byte0_names[] = {
> @@ -244,12 +249,6 @@ static const struct value_name_pair
> variable_length_arr[] = {
> };
> #define VARIABLE_LENGTH_SZ ARRAY_SIZE(variable_length_arr)
>
> -struct sa_name_list {
> - int cmd;
> - const struct value_name_pair *arr;
> - int arr_sz;
> -};
> -
> static struct sa_name_list sa_names_arr[] = {
> {VARIABLE_LENGTH_CMD, variable_length_arr, VARIABLE_LENGTH_SZ},
> {MAINTENANCE_IN, maint_in_arr, MAINT_IN_SZ},
> @@ -267,6 +266,28 @@ static struct sa_name_list sa_names_arr[] = {
> };
> #define SA_NAME_LIST_SZ ARRAY_SIZE(sa_names_arr)
We can delete SA_NAME_LIST_SZ because we define it out of
CONFIG_SCSI_CONSTANTS.
The others look good.
Thanks,
Yoshihiro YUNOMAE
> +#else /* ifndef CONFIG_SCSI_CONSTANTS */
> +static const char * cdb_byte0_names[] = NULL;
> +
> +static struct sa_name_list sa_names_arr[] = {
> + {VARIABLE_LENGTH_CMD, NULL, 0},
> + {MAINTENANCE_IN, NULL, 0},
> + {MAINTENANCE_OUT, NULL, 0},
> + {PERSISTENT_RESERVE_IN, NULL, 0},
> + {PERSISTENT_RESERVE_OUT, NULL, 0},
> + {SERVICE_ACTION_IN_12, NULL, 0},
> + {SERVICE_ACTION_OUT_12, NULL, 0},
> + {SERVICE_ACTION_BIDIRECTIONAL, NULL, 0},
> + {SERVICE_ACTION_IN_16, NULL, 0},
> + {SERVICE_ACTION_OUT_16, NULL, 0},
> + {THIRD_PARTY_COPY_IN, NULL, 0},
> + {THIRD_PARTY_COPY_OUT, NULL, 0},
> + {0, NULL, 0},
> +};
> +#endif /* CONFIG_SCSI_CONSTANTS */
> +#define SA_NAME_LIST_SZ ARRAY_SIZE(sa_names_arr)
> +#define CDB_BYTE0_NAMES_SZ ARRAY_SIZE(cdb_byte0_names)
> +
> static bool scsi_opcode_sa_name(int cmd, int service_action,
> const char **sa_name)
> {
> @@ -316,11 +337,14 @@ static void print_opcode_name(unsigned char * cdbp, int
> cdb_len)
>
> if (!scsi_opcode_sa_name(cdb0, sa, &name)) {
> if (cdb0 < 0xc0) {
> - name = cdb_byte0_names[cdb0];
> - if (name)
> - printk("%s", name);
> - else
> - printk("cdb[0]=0x%x (reserved)", cdb0);
> + if (CDB_BYTE0_NAMES_SZ) {
> + name = cdb_byte0_names[cdb0];
> + if (name)
> + printk("%s", name);
> + else
> + printk("cdb[0]=0x%x (reserved)", cdb0);
> + } else
> + printk("cdb[0]=0x%x", cdb0);
> } else
> printk("cdb[0]=0x%x (vendor)", cdb0);
> } else {
> @@ -334,50 +358,6 @@ static void print_opcode_name(unsigned char * cdbp, int
> cdb_len)
> }
> }
>
> -#else /* ifndef CONFIG_SCSI_CONSTANTS */
> -
> -static void print_opcode_name(unsigned char * cdbp, int cdb_len)
> -{
> - int sa, len, cdb0;
> -
> - cdb0 = cdbp[0];
> - switch(cdb0) {
> - case VARIABLE_LENGTH_CMD:
> - len = scsi_varlen_cdb_length(cdbp);
> - if (len < 10) {
> - printk("short opcode=0x%x command, len=%d "
> -"ext_len=%d", cdb0, len, cdb_len);
> - break;
> - }
> - sa = (cdbp[8] << 8) + cdbp[9];
> - printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
> - if (len != cdb_len)
> - printk(", in_cdb_len=%d, ext_len=%d", len, cdb_len);
> - break;
> - case MAINTENANCE_IN:
> - case MAINTENANCE_OUT:
> - case PERSISTENT_RESERVE_IN:
> - case PERSISTENT_RESERVE_OUT:
> - case SERVICE_ACTION_IN_12:
> - case SERVICE_ACTION_OUT_12:
> - case SERVICE_ACTION_BIDIRECTIONAL:
> - case SERVICE_ACTION_IN_16:
> - case SERVICE_ACTION_OUT_16:
> - case THIRD_PARTY_COPY_IN:
> - case THIRD_PARTY_COPY_OUT:
> - sa = cdbp[1] & 0x1f;
> - printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
> - break;
> - default:
> - if (cdb0 < 0xc0)
> - printk("cdb[0]=0x%x", cdb0);
> - else
> - printk("cdb[0]=0x%x (vendor)", cdb0);
> - break;
> - }
> -}
> -#endif
> -
> void __scsi_print_command(
Re: [PATCH 14/20] scsi: use local buffer for printing CDB
(2014/09/03 19:06), Hannes Reinecke wrote:
> The CDB needs to be printed in one line (ie with one printk
> statement) to avoid the individual bytes to be broken up
> under high load.
> As using individual printk() statements here would lead to
> unnecessary complicated code and needs the stack space to
> hold the format string we should be using a local buffer
> here. If the CDB is longer than the provided buffer
> it will be printed in several lines.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/ch.c| 5 +--
> drivers/scsi/constants.c | 82
>
> drivers/scsi/sr_ioctl.c | 9 --
> include/scsi/scsi_dbg.h | 2 +-
> 4 files changed, 65 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index eea94a9..f0df02e 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -189,6 +189,7 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd,
> {
> int errno, retries = 0, timeout, result;
> struct scsi_sense_hdr sshdr;
> + char buf[80];
This patch introduces some magic numbers like here,
but we had better define those.
Thanks,
Yoshihiro YUNOMAE
> timeout = (cmd[0] == INITIALIZE_ELEMENT_STATUS)
> ? timeout_init : timeout_move;
> @@ -196,8 +197,8 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd,
>retry:
> errno = 0;
> if (debug) {
> - DPRINTK("command: ");
> - __scsi_print_command(cmd);
> + __scsi_print_command(cmd, buf, 80);
> + DPRINTK("command: %s", buf);
> }
>
> result = scsi_execute_req(ch->device, cmd, direction, buffer,
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 6076969..5486816 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -322,19 +322,20 @@ static bool scsi_opcode_sa_name(int cmd, int
> service_action,
> return true;
> }
>
> -/* attempt to guess cdb length if cdb_len==0 . No trailing linefeed. */
> -static void print_opcode_name(unsigned char * cdbp, int cdb_len)
> +/* attempt to guess cdb length if cdb_len==0 */
> +static int print_opcode_name(unsigned char * cdbp, int cdb_len,
> + char *buf, int buf_len)
> {
> - int sa, len, cdb0;
> + int sa, len, cdb0, off = 0;
> const char *cdb_name = NULL, *sa_name = NULL;
>
> cdb0 = cdbp[0];
> if (cdb0 == VARIABLE_LENGTH_CMD) {
> len = scsi_varlen_cdb_length(cdbp);
> if (len < 10) {
> - printk("short variable length command, "
> -"len=%d ext_len=%d", len, cdb_len);
> - return;
> + return scnprintf(buf, buf_len,
> + "short variable length command, "
> + "len=%d ext_len=%d", len, cdb_len);
> }
> sa = (cdbp[8] << 8) + cdbp[9];
> } else {
> @@ -344,54 +345,81 @@ static void print_opcode_name(unsigned char * cdbp, int
> cdb_len)
>
> if (!scsi_opcode_sa_name(cdb0, sa, &cdb_name, &sa_name)) {
> if (cdb_name)
> - printk("%s", cdb_name);
> + off = scnprintf(buf, buf_len, "%s", cdb_name);
> else if (cdb0 > 0xc0)
> - printk("cdb[0]=0x%x (vendor)", cdb0);
> + off = scnprintf(buf, buf_len,
> + "cdb[0]=0x%x (vendor)", cdb0);
> else if (cdb0 > 0x60 && cdb0 < 0x7e)
> - printk("cdb[0]=0x%x (reserved)", cdb0);
> + off = scnprintf(buf, buf_len,
> + "cdb[0]=0x%x (reserved)", cdb0);
> else
> - printk("cdb[0]=0x%x", cdb0);
> + off = scnprintf(buf, buf_len, "cdb[0]=0x%x", cdb0);
> } else {
> if (sa_name)
> - printk("%s", sa_name);
> + off = scnprintf(buf, buf_len, "%s", sa_name);
> else if (cdb_name)
> - printk("%s, sa=0x%x", cdb_name, sa);
> + off = scnprintf(buf, buf_len,
> + "%s, sa=0x%x", cdb_name, sa);
> else
> - printk("cdb[0]=0x%x, sa=0x%x", cdb0, sa);
> + off = scnprintf(buf, buf_len,
> + "cdb[0]=0x%x, sa=0x%x", cdb0, sa);
>
> if (cdb_len > 0 && len != cdb_len)
> - printk(", in_cdb_len=%d, ext_len=%d", len, cdb_len);
> + off += scnprintf(buf + off, buf_len - off,
> + ", in_cdb_len=%d, ext_len=%d",
> + len, cdb_len);
> }
> + return off;
> }
>
> -void __scsi_print_command(unsigned char *cdb)
> +void __scsi_print_command(unsig
Re: [PATCH 16/20] scsi: separate out scsi_retval_string()
(2014/09/03 19:06), Hannes Reinecke wrote:
> Implement scsi_retval_string() to simplify logging.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/constants.c | 28
> drivers/scsi/scsi.c | 34 ++
> include/scsi/scsi_dbg.h | 1 +
> 3 files changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 5486816..85d2da0 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -1426,6 +1426,34 @@ void scsi_print_sense(struct scsi_cmnd *cmd)
> EXPORT_SYMBOL(scsi_print_sense);
>
> #ifdef CONFIG_SCSI_CONSTANTS
> +static const struct error_info internal_retval_table[] =
> +{
> + { NEEDS_RETRY, "NEEDS_RETRY " },
> + { SUCCESS, "SUCCESS " },
> + { FAILED, "FAILED " },
> + { QUEUED, "QUEUED " },
> + { SOFT_ERROR, "SOFT_ERROR " },
> + { ADD_TO_MLQUEUE, "ADD_TO_MLQUEUE " },
> + { TIMEOUT_ERROR, "TIMEOUT " },
> + { SCSI_RETURN_NOT_HANDLED, "NOT_HANDLED " },
> + { FAST_IO_FAIL, "FAST_IO_FAIL " },
We don't need to add space in the last of strings, I think.
In scsi_log_completion(), the messages inserts line feeds after the
strings.
> +};
> +#endif
> +
> +const char *
> +scsi_retval_string(unsigned int ret)
> +{
> +#ifdef CONFIG_SCSI_CONSTANTS
> + int i;
> +
> + for (i = 0; internal_retval_table[i].text; i++)
> + if (internal_retval_table[i].code12 == ret)
> + return internal_retval_table[i].text;
> +#endif
> + return NULL;
> +}
> +
> +#ifdef CONFIG_SCSI_CONSTANTS
>
> static const char * const hostbyte_table[]={
> "DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT",
> "DID_BAD_TARGET",
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 8954036..a1944c8 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -566,35 +566,13 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int
> disposition)
> SCSI_LOG_MLCOMPLETE_BITS);
> if (((level > 0) && (cmd->result || disposition != SUCCESS)) ||
> (level > 1)) {
> - scmd_printk(KERN_INFO, cmd, "Done: ");
> if (level > 2)
> - printk("0x%p ", cmd);
> - /*
> - * Dump truncated values, so we usually fit within
> - * 80 chars.
> - */
> - switch (disposition) {
> - case SUCCESS:
> - printk("SUCCESS\n");
> - break;
> - case NEEDS_RETRY:
> - printk("RETRY\n");
> - break;
> - case ADD_TO_MLQUEUE:
> - printk("MLQUEUE\n");
> - break;
> - case FAILED:
> - printk("FAILED\n");
> - break;
> - case TIMEOUT_ERROR:
> - /*
> - * If called via scsi_times_out.
> - */
> - printk("TIMEOUT\n");
> - break;
> - default:
> - printk("UNKNOWN\n");
> - }
> + scmd_printk(KERN_INFO, cmd,
> + "Done: 0x%p %s\n", cmd,
> + scsi_retval_string(disposition));
> + else
> + scmd_printk(KERN_INFO, cmd, "Done: %s",
We had better add "\n" in this last strings to indicate the end of line.
Structured printk automatically outputs the message in atomic,
but adding "\n" becomes more readable.
Thanks,
Yoshihiro YUNOMAE
> + scsi_retval_string(disposition));
> scsi_print_result(cmd);
> scsi_print_command(cmd);
> if (status_byte(cmd->result) & CHECK_CONDITION)
> diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h
> index 5020e5e..1030cc1 100644
> --- a/include/scsi/scsi_dbg.h
> +++ b/include/scsi/scsi_dbg.h
> @@ -19,6 +19,7 @@ extern void __scsi_print_sense(struct scsi_device *, const
> char *name,
> int sense_len);
> extern void scsi_show_result(int);
> extern void scsi_print_result(struct scsi_cmnd *);
> +extern const char *scsi_retval_string(unsigned int);
> extern const char *scsi_sense_key_string(unsigned char);
> extern const char *scsi_extd_sense_format(unsigned char, unsigned char,
> const char **);
>
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yo
Re: [SCSI] be2iscsi: adding functionality to change network settings using iscsiadm
On 09/04/2014 05:27 AM, Dan Carpenter wrote:
> Hello Mike Christie,
>
> The patch 0e43895ec1f4: "[SCSI] be2iscsi: adding functionality to
> change network settings using iscsiadm" from Apr 3, 2012, leads to
> the following static checker warning:
>
> drivers/scsi/be2iscsi/be_mgmt.c:945 mgmt_static_ip_modify()
> error: 'ip_param->len' from user is not capped properly
>
> drivers/scsi/be2iscsi/be_mgmt.c
>940 req->ip_params.ip_record.ip_addr.size_of_structure =
>941 sizeof(struct be_ip_addr_subnet_format);
>942 req->ip_params.ip_record.ip_addr.ip_type = ip_type;
>943
>944 if (ip_action == IP_ACTION_ADD) {
>945 memcpy(req->ip_params.ip_record.ip_addr.addr,
> ip_param->value,
>946 ip_param->len);
>947
>948 if (subnet_param)
>949
> memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
>950 subnet_param->value,
> subnet_param->len);
> ^
>951 } else {
>952 memcpy(req->ip_params.ip_record.ip_addr.addr,
>953 if_info->ip_addr.addr, ip_param->len);
>^
>954
>955 memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
>956 if_info->ip_addr.subnet_mask, ip_param->len);
>
>957 }
>
> These memcpy()s can overflow. It seems root only but it makes the
> static checker complain.
>
> One call tree is:
>
> beiscsi_set_static_ip() <- gets iface_ip.
> -> mgmt_set_ip()
> -> mgmt_static_ip_modify()
>
Jay, I made the attached patch to fix these issues plus one more I
found. I am still waiting on getting systems at work. Could you have
your people test it?
diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c
index 8478506..681d4e8 100644
--- a/drivers/scsi/be2iscsi/be_mgmt.c
+++ b/drivers/scsi/be2iscsi/be_mgmt.c
@@ -943,17 +943,20 @@ mgmt_static_ip_modify(struct beiscsi_hba *phba,
if (ip_action == IP_ACTION_ADD) {
memcpy(req->ip_params.ip_record.ip_addr.addr, ip_param->value,
- ip_param->len);
+ sizeof(req->ip_params.ip_record.ip_addr.addr));
if (subnet_param)
memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
- subnet_param->value, subnet_param->len);
+ subnet_param->value,
+ sizeof(req->ip_params.ip_record.ip_addr.subnet_mask));
} else {
memcpy(req->ip_params.ip_record.ip_addr.addr,
- if_info->ip_addr.addr, ip_param->len);
+ if_info->ip_addr.addr,
+ sizeof(req->ip_params.ip_record.ip_addr.addr));
memcpy(req->ip_params.ip_record.ip_addr.subnet_mask,
- if_info->ip_addr.subnet_mask, ip_param->len);
+ if_info->ip_addr.subnet_mask,
+ sizeof(req->ip_params.ip_record.ip_addr.subnet_mask));
}
rc = mgmt_exec_nonemb_cmd(phba, &nonemb_cmd, NULL, 0);
@@ -981,7 +984,7 @@ static int mgmt_modify_gateway(struct beiscsi_hba *phba, uint8_t *gt_addr,
req->action = gtway_action;
req->ip_addr.ip_type = BE2_IPV4;
- memcpy(req->ip_addr.addr, gt_addr, param_len);
+ memcpy(req->ip_addr.addr, gt_addr, sizeof(req->ip_addr.addr));
return mgmt_exec_nonemb_cmd(phba, &nonemb_cmd, NULL, 0);
}
Re: Off topic: Test suite for LLDs
On Wed, Sep 03, 2014 at 11:11:54PM -0600, Sathya Prakash Veerichetty wrote: > Hello folks, > I would like to know whether we have any standard test suite for > validating a low level driver by executing all the interfaces the low > level driver registers with SML? Unfortunately there is none. But if there was one it would be highly on topic here. -- 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 17/20] scsi: separate out scsi_host_hostbyte() and scsi_show_driverbyte()
This patch looks good.
Thanks,
Yoshihiro YUNOMAE
(2014/09/03 19:06), Hannes Reinecke wrote:
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/constants.c | 56
> +---
> include/scsi/scsi_dbg.h | 2 ++
> 2 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 85d2da0..630e272 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -1468,31 +1468,63 @@ static const char * const driverbyte_table[]={
> "DRIVER_INVALID", "DRIVER_TIMEOUT", "DRIVER_HARD", "DRIVER_SENSE"};
> #define NUM_DRIVERBYTE_STRS ARRAY_SIZE(driverbyte_table)
>
> -void scsi_show_result(int result)
> +#endif
> +
> +const char *scsi_show_hostbyte(int result)
> {
> + const char *hb_string = NULL;
> +#ifdef CONFIG_SCSI_CONSTANTS
> int hb = host_byte(result);
> - int db = driver_byte(result);
>
> - printk("Result: hostbyte=%s driverbyte=%s\n",
> -(hb < NUM_HOSTBYTE_STRS ? hostbyte_table[hb] : "invalid"),
> -(db < NUM_DRIVERBYTE_STRS ? driverbyte_table[db] : "invalid"));
> + if (hb < NUM_HOSTBYTE_STRS)
> + hb_string = hostbyte_table[hb];
> +#endif
> + return hb_string;
> }
> +EXPORT_SYMBOL(scsi_show_hostbyte);
>
> -#else
> +const char *scsi_show_driverbyte(int result)
> +{
> + const char *db_string = NULL;
> +#ifdef CONFIG_SCSI_CONSTANTS
> + int db = driver_byte(result);
> +
> + if (db < NUM_DRIVERBYTE_STRS)
> + db_string = driverbyte_table[db];
> +#endif
> + return db_string;
> +}
> +EXPORT_SYMBOL(scsi_show_driverbyte);
>
> void scsi_show_result(int result)
> {
> - printk("Result: hostbyte=0x%02x driverbyte=0x%02x\n",
> -host_byte(result), driver_byte(result));
> -}
> + const char *hb_string = scsi_show_hostbyte(result);
> + const char *db_string = scsi_show_driverbyte(result);
>
> -#endif
> + if (hb_string || db_string)
> + printk("Result: hostbyte=%s driverbyte=%s\n",
> +hb_string ? hb_string : "invalid",
> +db_string ? db_string : "invalid");
> + else
> + printk("Result: hostbyte=0x%02x driverbyte=0x%02x\n",
> +host_byte(result), driver_byte(result));
> +}
> EXPORT_SYMBOL(scsi_show_result);
>
>
> void scsi_print_result(struct scsi_cmnd *cmd)
> {
> - scmd_printk(KERN_INFO, cmd, " ");
> - scsi_show_result(cmd->result);
> + const char *hb_string = scsi_show_hostbyte(cmd->result);
> + const char *db_string = scsi_show_driverbyte(cmd->result);
> +
> + if (hb_string || db_string)
> + scmd_printk(KERN_INFO, cmd,
> + "Result: hostbyte=%s driverbyte=%s",
> + hb_string ? hb_string : "invalid",
> + db_string ? db_string : "invalid");
> + else
> + scmd_printk(KERN_INFO, cmd,
> + "Result: hostbyte=0x%02x driverbyte=0x%02x",
> + host_byte(cmd->result), driver_byte(cmd->result));
> }
> EXPORT_SYMBOL(scsi_print_result);
> diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h
> index 1030cc1..a3170a5 100644
> --- a/include/scsi/scsi_dbg.h
> +++ b/include/scsi/scsi_dbg.h
> @@ -19,6 +19,8 @@ extern void __scsi_print_sense(struct scsi_device *, const
> char *name,
> int sense_len);
> extern void scsi_show_result(int);
> extern void scsi_print_result(struct scsi_cmnd *);
> +extern const char *scsi_show_hostbyte(int);
> +extern const char *scsi_show_driverbyte(int);
> extern const char *scsi_retval_string(unsigned int);
> extern const char *scsi_sense_key_string(unsigned char);
> extern const char *scsi_extd_sense_format(unsigned char, unsigned char,
>
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae...@hitachi.com
--
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 18/20] scsi: remove scsi_show_result()
This patch looks good.
Thanks,
Yoshihiro YUNOMAE
(2014/09/03 19:06), Hannes Reinecke wrote:
> scsi_show_result() was only ever used in one place in sd.c.
> So open-code scsi_show_result() in sd.c and remove it from
> constants.c.
>
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/constants.c | 16 -
> drivers/scsi/sd.c| 62
> +---
> include/scsi/scsi_dbg.h | 1 -
> 3 files changed, 37 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 630e272..d7516c3 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -1496,22 +1496,6 @@ const char *scsi_show_driverbyte(int result)
> }
> EXPORT_SYMBOL(scsi_show_driverbyte);
>
> -void scsi_show_result(int result)
> -{
> - const char *hb_string = scsi_show_hostbyte(result);
> - const char *db_string = scsi_show_driverbyte(result);
> -
> - if (hb_string || db_string)
> - printk("Result: hostbyte=%s driverbyte=%s\n",
> -hb_string ? hb_string : "invalid",
> -db_string ? db_string : "invalid");
> - else
> - printk("Result: hostbyte=0x%02x driverbyte=0x%02x\n",
> -host_byte(result), driver_byte(result));
> -}
> -EXPORT_SYMBOL(scsi_show_result);
> -
> -
> void scsi_print_result(struct scsi_cmnd *cmd)
> {
> const char *hb_string = scsi_show_hostbyte(cmd->result);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index aac267a..f96e3f9 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -116,7 +116,7 @@ static int sd_eh_action(struct scsi_cmnd *, int);
> static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
> static void scsi_disk_release(struct device *cdev);
> static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);
> -static void sd_print_result(struct scsi_disk *, int);
> +static void sd_print_result(struct scsi_disk *, const char *, int);
>
> static DEFINE_SPINLOCK(sd_index_lock);
> static DEFINE_IDA(sd_index_ida);
> @@ -1479,7 +1479,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> }
>
> if (res) {
> - sd_print_result(sdkp, res);
> + sd_print_result(sdkp, "SYNCHRONIZE CACHE failed", res);
>
> if (driver_byte(res) & DRIVER_SENSE)
> sd_print_sense_hdr(sdkp, &sshdr);
> @@ -1820,8 +1820,8 @@ sd_spinup_disk(struct scsi_disk *sdkp)
> /* no sense, TUR either succeeded or failed
>* with a status error */
> if(!spintime && !scsi_status_is_good(the_result)) {
> - sd_printk(KERN_NOTICE, sdkp, "Unit Not
> Ready\n");
> - sd_print_result(sdkp, the_result);
> + sd_print_result(sdkp, "Unit Not Ready",
> + the_result);
> }
> break;
> }
> @@ -1937,11 +1937,13 @@ static int sd_read_protection_type(struct scsi_disk
> *sdkp, unsigned char *buffer
> return ret;
> }
>
> -static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device
> *sdp,
> - struct scsi_sense_hdr *sshdr, int sense_valid,
> - int the_result)
> +static void read_capacity_error(struct scsi_disk *sdkp,
> + struct scsi_sense_hdr *sshdr, int sense_valid,
> + const char *msg, int the_result)
> {
> - sd_print_result(sdkp, the_result);
> + struct scsi_device *sdp = sdkp->device;
> +
> + sd_print_result(sdkp, msg, the_result);
> if (driver_byte(the_result) & DRIVER_SENSE)
> sd_print_sense_hdr(sdkp, sshdr);
> else
> @@ -1970,9 +1972,9 @@ static void read_capacity_error(struct scsi_disk *sdkp,
> struct scsi_device *sdp,
>
> #define READ_CAPACITY_RETRIES_ON_RESET 10
>
> -static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
> - unsigned char *buffer)
> +static int read_capacity_16(struct scsi_disk *sdkp, unsigned char *buffer)
> {
> + struct scsi_device *sdp = sdkp->device;
> unsigned char cmd[16];
> struct scsi_sense_hdr sshdr;
> int sense_valid = 0;
> @@ -2022,8 +2024,8 @@ static int read_capacity_16(struct scsi_disk *sdkp,
> struct scsi_device *sdp,
> } while (the_result && retries);
>
> if (the_result) {
> - sd_printk(KERN_NOTICE, sdkp, "READ CAPACITY(16) failed\n");
> - read_capacity_error(sdkp, sdp, &sshdr, sense_valid, the_result);
> + read_capacity_error(sdkp, &sshdr, sense_valid,
> + "READ CAPACITY(16) failed", the_result);
> return -EINVAL;
> }
>
> @@ -2066,9 +2068,9 @@ static int read
Re: [PATCH 20/20] scsi_error: format abort error message
(2014/09/03 19:06), Hannes Reinecke wrote:
> Decode the return value if the command abort failed.
>
> Suggested-by: Robert Elliot
> Signed-off-by: Hannes Reinecke
> ---
> drivers/scsi/scsi_error.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 8a6d382..eca63b2 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -157,8 +157,8 @@ scmd_eh_abort_handler(struct work_struct *work)
> } else {
> SCSI_LOG_ERROR_RECOVERY(3,
> scmd_printk(KERN_INFO, scmd,
> - "scmd %p abort failed, rtn %d\n",
> - scmd, rtn));
> + "scmd %p abort failed, rtn %s\n",
> + scmd, scsi_retval_string(rtn)));
If CONFIG_SCSI_CONSTANTS is disabled, scsi_retval_string() returns NULL.
So, if so, we had better output the raw value here.
Thanks,
Yoshihiro YUNOMAE
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae...@hitachi.com
--
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_debug: deadlock between completions and surprise module removal
Can I get another review for this one?
On Sun, Aug 31, 2014 at 07:09:59PM -0400, Douglas Gilbert wrote:
> A deadlock has been reported when the completion
> of SCSI commands (simulated by a timer) was surprised
> by a module removal. This patch removes one half of
> the offending locks around timer deletions. This fix
> is applied both to stop_all_queued() which is were
> the deadlock was discovered and stop_queued_cmnd()
> which has very similar logic.
>
> This patch should be applied both to the lk 3.17 tree
> and Christoph's drivers-for-3.18 tree.
>
> Tested-and-reported-by: Milan Broz
> Signed-off-by: Douglas Gilbert
> --- a/drivers/scsi/scsi_debug.c 2014-08-26 13:24:51.646948507 -0400
> +++ b/drivers/scsi/scsi_debug.c 2014-08-30 18:04:54.589226679 -0400
> @@ -2743,6 +2743,13 @@ static int stop_queued_cmnd(struct scsi_
> if (test_bit(k, queued_in_use_bm)) {
> sqcp = &queued_arr[k];
> if (cmnd == sqcp->a_cmnd) {
> + devip = (struct sdebug_dev_info *)
> + cmnd->device->hostdata;
> + if (devip)
> + atomic_dec(&devip->num_in_q);
> + sqcp->a_cmnd = NULL;
> + spin_unlock_irqrestore(&queued_arr_lock,
> +iflags);
> if (scsi_debug_ndelay > 0) {
> if (sqcp->sd_hrtp)
> hrtimer_cancel(
> @@ -2755,18 +2762,13 @@ static int stop_queued_cmnd(struct scsi_
> if (sqcp->tletp)
> tasklet_kill(sqcp->tletp);
> }
> - __clear_bit(k, queued_in_use_bm);
> - devip = (struct sdebug_dev_info *)
> - cmnd->device->hostdata;
> - if (devip)
> - atomic_dec(&devip->num_in_q);
> - sqcp->a_cmnd = NULL;
> - break;
> + clear_bit(k, queued_in_use_bm);
> + return 1;
> }
> }
> }
> spin_unlock_irqrestore(&queued_arr_lock, iflags);
> - return (k < qmax) ? 1 : 0;
> + return 0;
> }
>
> /* Deletes (stops) timers or tasklets of all queued commands */
> @@ -2782,6 +2784,13 @@ static void stop_all_queued(void)
> if (test_bit(k, queued_in_use_bm)) {
> sqcp = &queued_arr[k];
> if (sqcp->a_cmnd) {
> + devip = (struct sdebug_dev_info *)
> + sqcp->a_cmnd->device->hostdata;
> + if (devip)
> + atomic_dec(&devip->num_in_q);
> + sqcp->a_cmnd = NULL;
> + spin_unlock_irqrestore(&queued_arr_lock,
> +iflags);
> if (scsi_debug_ndelay > 0) {
> if (sqcp->sd_hrtp)
> hrtimer_cancel(
> @@ -2794,12 +2803,8 @@ static void stop_all_queued(void)
> if (sqcp->tletp)
> tasklet_kill(sqcp->tletp);
> }
> - __clear_bit(k, queued_in_use_bm);
> - devip = (struct sdebug_dev_info *)
> - sqcp->a_cmnd->device->hostdata;
> - if (devip)
> - atomic_dec(&devip->num_in_q);
> - sqcp->a_cmnd = NULL;
> + clear_bit(k, queued_in_use_bm);
> + spin_lock_irqsave(&queued_arr_lock, iflags);
> }
> }
> }
---end quoted text---
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/19] lpfc 10.4.8000.0: Update lpfc version to driver version 10.4.8000.0
I've applied the series, but for next time can you make sure to follow the proper format: - remove the version number in every subject line - patches you resend from an original author should be unchanged, except that the From: lines moves into the mail body - patches that you send on with your maintainer hat on should be signed off by you, not just reviewed. not strictly required, but making my life a lot easier would be if all patches are sent by reply to the original mail. git-send-email does this, and it seems like the Emulex division supporting be2scsi has found a way to use it with the corporate email servers. -- 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: Don't store LUN bits in CDB[1] for USB mass-storage devices
Thanks, applied. -- 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] bnx2fc: fix incorrect DMA memory mapping in bnx2fc_unmap_sg_list()
Thanks, applied. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Drivers: scsi: storvsc: Get rid of warning messages
Looks good to me. Olaf, Hannes - can I get another review for this (and the older hyperv scanning patch set)? -- 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 08/20] scsi: use 'bool' as return value for scsi_normalize_sense()
On 09/05/2014 02:51 AM, Yoshihiro YUNOMAE wrote:
> (2014/09/03 19:06), Hannes Reinecke wrote:
>> Convert scsi_normalize_sense() and frieds to return 'bool'
>> instead of an integer.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> drivers/scsi/scsi_error.c | 14 +++---
>> drivers/scsi/scsi_lib.c | 2 +-
>> include/scsi/scsi_eh.h| 12 ++--
>> 3 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 6c99624..8a6d382 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -2408,20 +2408,20 @@ EXPORT_SYMBOL(scsi_reset_provider);
>>* responded to a SCSI command with the CHECK_CONDITION status.
>>*
>>* Return value:
>> - * 1 if valid sense data information found, else 0;
>> + * true if valid sense data information found, else false;
>>*/
>> -int scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>> - struct scsi_sense_hdr *sshdr)
>> +bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>> + struct scsi_sense_hdr *sshdr)
>> {
>> if (!sense_buffer || !sb_len)
>> -return 0;
>> +return false;
>>
>> memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
>>
>> sshdr->response_code = (sense_buffer[0] & 0x7f);
>>
>> if (!scsi_sense_valid(sshdr))
>> -return 0;
>> +return false;
>>
>> if (sshdr->response_code >= 0x72) {
>> /*
>> @@ -2451,11 +2451,11 @@ int scsi_normalize_sense(const u8 *sense_buffer, int
>> sb_len,
>> }
>> }
>>
>> -return 1;
>> +return true;
>> }
>> EXPORT_SYMBOL(scsi_normalize_sense);
>>
>> -int scsi_command_normalize_sense(struct scsi_cmnd *cmd,
>> +bool scsi_command_normalize_sense(struct scsi_cmnd *cmd,
>> struct scsi_sense_hdr *sshdr)
>> {
>> return scsi_normalize_sense(cmd->sense_buffer,
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index de178f7..8cad004 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -830,7 +830,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
>> int good_bytes)
>> struct request *req = cmd->request;
>> int error = 0;
>> struct scsi_sense_hdr sshdr;
>> -int sense_valid = 0;
>> +bool sense_valid = false;
>> int sense_deferred = 0;
>> enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
>>ACTION_DELAYED_RETRY} action;
>> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
>> index 06a8790..1427a66 100644
>> --- a/include/scsi/scsi_eh.h
>> +++ b/include/scsi/scsi_eh.h
>> @@ -27,7 +27,7 @@ struct scsi_sense_hdr {/* See SPC-3 section
>> 4.5 */
>> u8 additional_length; /* always 0 for fixed sense format */
>> };
>>
>> -static inline int scsi_sense_valid(struct scsi_sense_hdr *sshdr)
>> +static inline bool scsi_sense_valid(struct scsi_sense_hdr *sshdr)
>> {
>> if (!sshdr)
>> return 0;
>
> Should be "return false;"
>
Bah. You are correct.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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 16/20] scsi: separate out scsi_retval_string()
On 09/05/2014 04:04 AM, Yoshihiro YUNOMAE wrote:
> (2014/09/03 19:06), Hannes Reinecke wrote:
>> Implement scsi_retval_string() to simplify logging.
>>
>> Signed-off-by: Hannes Reinecke
>> ---
>> drivers/scsi/constants.c | 28
>> drivers/scsi/scsi.c | 34 ++
>> include/scsi/scsi_dbg.h | 1 +
>> 3 files changed, 35 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
>> index 5486816..85d2da0 100644
>> --- a/drivers/scsi/constants.c
>> +++ b/drivers/scsi/constants.c
>> @@ -1426,6 +1426,34 @@ void scsi_print_sense(struct scsi_cmnd *cmd)
>> EXPORT_SYMBOL(scsi_print_sense);
>>
>> #ifdef CONFIG_SCSI_CONSTANTS
>> +static const struct error_info internal_retval_table[] =
>> +{
>> +{ NEEDS_RETRY, "NEEDS_RETRY " },
>> +{ SUCCESS, "SUCCESS " },
>> +{ FAILED, "FAILED " },
>> +{ QUEUED, "QUEUED " },
>> +{ SOFT_ERROR, "SOFT_ERROR " },
>> +{ ADD_TO_MLQUEUE, "ADD_TO_MLQUEUE " },
>> +{ TIMEOUT_ERROR, "TIMEOUT " },
>> +{ SCSI_RETURN_NOT_HANDLED, "NOT_HANDLED " },
>> +{ FAST_IO_FAIL, "FAST_IO_FAIL " },
>
> We don't need to add space in the last of strings, I think.
> In scsi_log_completion(), the messages inserts line feeds after the
> strings.
>
>> +};
>> +#endif
>> +
>> +const char *
>> +scsi_retval_string(unsigned int ret)
>> +{
>> +#ifdef CONFIG_SCSI_CONSTANTS
>> +int i;
>> +
>> +for (i = 0; internal_retval_table[i].text; i++)
>> +if (internal_retval_table[i].code12 == ret)
>> +return internal_retval_table[i].text;
>> +#endif
>> +return NULL;
>> +}
>> +
>> +#ifdef CONFIG_SCSI_CONSTANTS
>>
>> static const char * const hostbyte_table[]={
>> "DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT",
>> "DID_BAD_TARGET",
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 8954036..a1944c8 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -566,35 +566,13 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int
>> disposition)
>> SCSI_LOG_MLCOMPLETE_BITS);
>> if (((level > 0) && (cmd->result || disposition != SUCCESS)) ||
>> (level > 1)) {
>> -scmd_printk(KERN_INFO, cmd, "Done: ");
>> if (level > 2)
>> -printk("0x%p ", cmd);
>> -/*
>> - * Dump truncated values, so we usually fit within
>> - * 80 chars.
>> - */
>> -switch (disposition) {
>> -case SUCCESS:
>> -printk("SUCCESS\n");
>> -break;
>> -case NEEDS_RETRY:
>> -printk("RETRY\n");
>> -break;
>> -case ADD_TO_MLQUEUE:
>> -printk("MLQUEUE\n");
>> -break;
>> -case FAILED:
>> -printk("FAILED\n");
>> -break;
>> -case TIMEOUT_ERROR:
>> -/*
>> - * If called via scsi_times_out.
>> - */
>> -printk("TIMEOUT\n");
>> -break;
>> -default:
>> -printk("UNKNOWN\n");
>> -}
>> +scmd_printk(KERN_INFO, cmd,
>> +"Done: 0x%p %s\n", cmd,
>> +scsi_retval_string(disposition));
>> +else
>> +scmd_printk(KERN_INFO, cmd, "Done: %s",
>
> We had better add "\n" in this last strings to indicate the end of line.
> Structured printk automatically outputs the message in atomic,
> but adding "\n" becomes more readable.
>
True, we should be adding the newline.
Although the reasoning is wrong; a newline instructs printk() to
actually ship out the message. Without a newline printk assumes
it'll be line continuation.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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
[RFC v2 4/6] cxgb4: use async probe
From: "Luis R. Rodriguez"
cxgb4 probe can take up to over 1 minute when the firmware is
is written and installed on the device, even after this the device
driver still does some device probing and can take quite a bit.
systemd will kill this driver when probe does take over 30 seconds,
use the asynch probe mechanism to circumvent this.
Cc: Tetsuo Handa
Cc: Joseph Salisbury
Cc: One Thousand Gnomes
Cc: Tim Gardner
Cc: Pierre Fersing
Cc: Andrew Morton
Cc: Oleg Nesterov
Cc: Benjamin Poirier
Cc: Greg Kroah-Hartman
Cc: Nagalakshmi Nandigama
Cc: Praveen Krishnamoorthy
Cc: Sreekanth Reddy
Cc: Abhijit Mahajan
Cc: Hariprasad S
Cc: Santosh Rastapur
Cc: Casey Leedom
Cc: mpt-fusionlinux@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Luis R. Rodriguez
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 18fb9c6..5f7d24a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -6794,6 +6794,7 @@ static struct pci_driver cxgb4_driver = {
.remove = remove_one,
.shutdown = remove_one,
.err_handler = &cxgb4_eeh,
+ .driver.async_probe = true,
};
static int __init cxgb4_init_module(void)
--
2.0.3
--
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
[RFC v2 2/6] driver-core: add driver async_probe support
From: "Luis R. Rodriguez" We now have two documented use cases for probing asynchronously: 0) since we bundle together driver init() and probe() systemd's new 30 second timeout has put a limit on the amount of time a driver probe routine can take, we need to enable drivers to complete probe gracefully 1) when a built-in driver takes a few seconds to initialize its delays can stall the overall boot process The built-in driver issues is pretty straight forward, and for that we just need to let the probing happen behind the scenes. The systemd issue is a bit more complex given the history of how it was identified and work arounds proposed and evaluated. The systemd issue was first identified first by Joseph when he bisected and found that Tetsuo Handa's commit 786235ee "kthread: make kthread_create() killable" modified kthread_create() to bail as soon as SIGKILL is received [0] [1]. This was found to cause some issues with some drivers and at times boot. There are other patches which could also enable the SIGKILL trigger on driver loading though: 70834d30 "usermodehelper: use UMH_WAIT_PROC consistently" b3449922 "usermodehelper: introduce umh_complete(sub_info)" d0bd587a "usermodehelper: implement UMH_KILLABLE" 9d944ef3 "usermodehelper: kill umh_wait, renumber UMH_* constants" 5b9bd473 "usermodehelper: call_usermodehelper() doesn't need do_exit()" 3e63a93b "kmod: introduce call_modprobe() helper" 1cc684ab "kmod: make __request_module() killable" All of these went in on 3.4 upstream, and were part of the fixes for CVE-2012-4398 [2] and documented more properly on Red Hat's bugzilla [3]. Any of these patches may contribute to having a module be properly killed now, but 786235ee is the latest in the series. For instance on SLE12 cxgb4 has been fond to get the SIGKILL even though SLE12 does not yet have 786235ee merged [4]. Joseph found that the systemd-udevd process sends SIGKILL to systemd's usage of kmod for module loading if probe on a driver takes over 30 seconds [5] [6]. When this happens probe will fail on any driver, but *iff* its probe path ends up using kthreads. Its why booting on some systems will fail if the driver happens to be a storage related driver. When helping debug the issue Tetsuo suggested fixing this issue by kmodifying kthread_create() to not leave upon SIGKILL immediately *unless* the source of the SIGKILL was the OOM, and actually wait for 10 seconds more before completing the kill [7] *unless* the source of the killer was OOM. This is not the only source of a kill, as noted above, this same issue is present on kernels without commit 786235ee. Additionally upon review of this patch Oleg rejected this change [8] and the discussion was punted out to systemd-devel to see if the default timeout could be increased from 30 seconds to 120 [9]. The opinion of the systemd maintainers was that the driver's behavior should be fixed [10]. Linus seems to agree [11], however more recently even networking drivers have been reported to fail on probe since just writing the firmware to a device and kicking it can take easy over 60 seconds [4]. Benjamim was able to trace the issues reported on cxgb4 down to the same systemd-udevd 30 second timeout [6]. Even if asynch firmware loading was used on the cxgb4 driver the driver would *still* hit other delays due to the way the driver is currently designed, fixing that will require a bit of time and until then some users are left with a completely dysfunctional device. Folks were a bit confused here though -- its not only module initialization which is being penalized, because the driver core will immediately trigger the driver's own bus probe routine if autoprobe is enabled each driver's probe routine must also complete within the same 30 second timeout. This means not only should driver's init complete within the set default systemd timeout of 30 seconds but so should the probe routine, and probe would obviously also have less time given that the timeout is for both the module's init() and its own bus' probe(). A few drivers fail to complete the bus' probe within 30 seconds, its not the init routine that takes long. The timeout seems to currently hit *iff* kthreads are used somehow on the driver's probe path. For example purposely breaking the e1000e driver by adding a 30 second timeout on the probe path does not let systemd kill it, however doing the same for iwlwifi triggers the kill, this is because this driver uses request_threaded_irq() and behind the scenes the kernel uses ktread_create() on __setup_irq() to handle the thread *iff* its not nested, these are drivers that set irq_set_nested_thread(irq, 1). Hannes Reinecke has implemented now a timeout modifier for systemd, however *systemd* still needs a way to gracefully annotate drivers with long probes instead of failing these drivers and at worst boot. On the kernel side of things we can circumvent the timeout by probing asynchronously on only drivers that need it. If a
[RFC v2 3/6] kthread: warn on kill signal if not OOM
From: "Luis R. Rodriguez" The new umh kill option has allowed kthreads to receive kill signals but they are generally accepting all sources of kill signals while the original motivation was to enable through the OOM from sending the kill. One particular user which has been found to send kill signals on kthreads is systemd, it does this upon a 30 second default timeout on loading modules. That timeout was in place under the assumption that some driver's init sequences were taking long. Since the kernel batches both init and probe together though its actually been the probe routines which take long. These should not be penalized, the kill would only happen if and only if the driver's probe routine ends up using kthreads somehow. To help with this we now have the async_probe flag for drivers but before we can amend drivers with this functionality we need to find them. This patch addresses that by avoiding the kill from any other source than the OOM killer -- for now. Users can provide a log output and it should be clear on the trace what probe / driver got the kill signal. This patch is based on Tetsuo's patch [0] to try to address the timeout issue, which in itself is based on Tetsuo's original patch to also address this months ago [1]. These patches just lacked addressing all other callers which would load modules for us. Although Oleg had rejected a similar change a while ago [2] its now clear what the source of the problem. A few solutions have been proposed, one of them was to allow the default systemd timeout to be modified, that change by Hannes Reinecke is now merged upstream on systemd, we still however need a non fatal way to deal with modules that take long and an easy way for us to find these modules. At least one proposal has been made for systemd but discussions on that approach hasn't gotten much traction [3] so we need to address this on the kernel, this will also be important for users of new kernels on old versions of systemd. [0] https://launchpadlibrarian.net/169657493/kthread-defer-leaving.patch [1] https://lkml.org/lkml/2014/7/29/284 [2] http://article.gmane.org/gmane.linux.kernel/1669604 [3] http://lists.freedesktop.org/archives/systemd-devel/2014-August/021852.html An example log output captured by purposely breaking the iwlwifi driver by using ssleep(33) on probe: [ 43.853997] iwlwifi going to sleep for 33 seconds [ 76.862975] iwlwifi done sleeping for 33 seconds [ 76.863880] iwlwifi :03:00.0: irq 34 for MSI/MSI-X [ 76.863961] [ cut here ] [ 76.864648] WARNING: CPU: 0 PID: 479 at kernel/kthread.c:308 kthread_create_on_node+0x1ea/0x200() [ 76.865309] Got SIGKILL but not from OOM, if this issue is on probe use .driver.async_probe [ 76.865974] Modules linked in: xfs libcrc32c x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel snd_hda_codec_realtek snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep aes_x86_64 uvcvideo glue_helper videobuf2_vmalloc lrw gf128mul snd_pcm ablk_helper iTCO_wdt rtsx_pci_ms videobuf2_memops videobuf2_core rtsx_pci_sdmmc v4l2_common mmc_core videodev snd_timer thinkpad_acpi memstick iTCO_vendor_support snd mei_me rtsx_pci cryptd iwlwifi(+) mei shpchp tpm_tis soundcore pcspkr joydev lpc_ich mfd_core serio_raw tpm btusb wmi i2c_i801 thermal intel_smartconnect ac battery processor dm_mod btrfs xor raid6_pq i915 i2c_algo_bit e1000e drm_kms_helper sr_mod crc32c_intel cdrom xhci_hcd drm video [ 76.869197] button sg [ 76.870035] CPU: 0 PID: 479 Comm: systemd-udevd Not tainted 3.17.0-rc3-25.g1474ea5-desktop+ #12 [ 76.870915] Hardware name: LENOVO 20AW000LUS/20AW000LUS, BIOS GLET43WW (1.18 ) 12/04/2013 [ 76.871801] 0009 8802133a3908 8173960f 8802133a3950 [ 76.872771] 8802133a3940 81072eed 8800c9004480 810c8fd0 [ 76.873693] 81a77845 8800c9d2abc0 8802133a39a0 [ 76.874620] Call Trace: [ 76.875522] [] dump_stack+0x4d/0x6f [ 76.876379] [] warn_slowpath_common+0x7d/0xa0 [ 76.877286] [] ? irq_thread_check_affinity+0xb0/0xb0 [ 76.878177] [] warn_slowpath_fmt+0x4c/0x50 [ 76.879048] [] ? irq_thread_check_affinity+0xb0/0xb0 [ 76.879898] [] kthread_create_on_node+0x1ea/0x200 [ 76.880765] [] ? enable_cpucache+0x4e/0xe0 [ 76.881617] [] __setup_irq+0x165/0x580 [ 76.882459] [] ? dma_generic_alloc_coherent+0x146/0x160 [ 76.883314] [] ? iwl_pcie_disable_ict+0x40/0x40 [iwlwifi] [ 76.884159] [] request_threaded_irq+0xcf/0x180 [ 76.885010] [] iwl_trans_pcie_alloc+0x35a/0x4b1 [iwlwifi] [ 76.885861] [] iwl_pci_probe+0x50/0x260 [iwlwifi] [ 76.886646] [] ? __pm_runtime_resume+0x4d/0x60 [ 76.887404] [] local_pci_probe+0x45/0xa0 [ 76.888155] [] ? pci_match_device+0xe5/0x110 [ 76.99] [] pci_device_probe+0xd9/0x130 [ 76.889646] [] driver_probe_device+0x12d/0x3e
[RFC v2 5/6] mptsas: use async probe
From: "Luis R. Rodriguez"
Its reported that mptsas can at times take over 30 seconds
to recognize SCSI storage devices [0], this is done on the
driver's probe path. Use the the new asynch probe to
circumvent systemd from killing this driver.
[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
Cc: Tetsuo Handa
Cc: Joseph Salisbury
Cc: One Thousand Gnomes
Cc: Tim Gardner
Cc: Pierre Fersing
Cc: Andrew Morton
Cc: Oleg Nesterov
Cc: Benjamin Poirier
Cc: Greg Kroah-Hartman
Cc: Nagalakshmi Nandigama
Cc: Praveen Krishnamoorthy
Cc: Sreekanth Reddy
Cc: Abhijit Mahajan
Cc: Hariprasad S
Cc: Santosh Rastapur
Cc: Casey Leedom
Cc: mpt-fusionlinux@avagotech.com
Cc: linux-scsi@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Luis R. Rodriguez
---
drivers/message/fusion/mptsas.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 0707fa2..6dfee95 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -5385,6 +5385,7 @@ static struct pci_driver mptsas_driver = {
.suspend= mptscsih_suspend,
.resume = mptscsih_resume,
#endif
+ .driver.async_probe = true,
};
static int __init
--
2.0.3
--
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
