Re: [PATCH v2] nvme-tcp: Check if request has started before processing it
On Wed, 2021-03-31 at 09:11 +0200, Hannes Reinecke wrote: > On 3/31/21 1:28 AM, Keith Busch wrote: > > On Tue, Mar 30, 2021 at 10:34:25AM -0700, Sagi Grimberg wrote: > > > > > > > > It is, but in this situation, the controller is sending a > > > > > second > > > > > completion that results in a use-after-free, which makes the > > > > > transport irrelevant. Unless there is some other flow (which > > > > > is > > > > > unclear > > > > > to me) that causes this which is a bug that needs to be fixed > > > > > rather > > > > > than hidden with a safeguard. > > > > > > > > > > > > > The kernel should not crash regardless of any network traffic > > > > that is > > > > sent to the system. It should not be possible to either > > > > intentionally > > > > of mistakenly contruct packets that will deny service in this > > > > way. > > > > > > This is not specific to nvme-tcp. I can build an rdma or pci > > > controller > > > that can trigger the same crash... I saw a similar patch from > > > Hannes > > > implemented in the scsi level, and not the individual scsi > > > transports.. > > > > If scsi wants this too, this could be made generic at the blk-mq > > level. > > We just need to make something like blk_mq_tag_to_rq(), but return > > NULL > > if the request isn't started. > > > > > I would also mention, that a crash is not even the scariest issue > > > that > > > we can see here, because if the request happened to be reused we > > > are > > > in the silent data corruption realm... > > > > If this does happen, I think we have to come up with some way to > > mitigate it. We're not utilizing the full 16 bits of the > > command_id, so > > maybe we can append something like a generation sequence number > > that can > > be checked for validity. > > > > ... which will be near impossible. > We can protect against crashing on invalid frames. > We can _not_ protect against maliciously crafted packets referencing > any > random _existing_ tag; that's what TLS is for. > > What we can do, though, is checking the 'state' field in the tcp > request, and only allow completions for commands which are in a state > allowing for completions. > > Let's see if I can whip up a patch. That would be great. BTW in the crash dump I am looking at now, it looks like pdu->command_id was zero in nvme_tcp_recv_data(), and blk_mq_tag_to_rq() returned a request struct that had not been used. So I think we do need to check that the tag was actually allocated. -Ewan > > Cheers, > > Hannes
Re: [PATCH] nvme: Export fast_io_fail_tmo to sysfs
On Wed, 2021-03-31 at 15:12 +0200, Daniel Wagner wrote:
> Commit 8c4dfea97f15 ("nvme-fabrics: reject I/O to offline device")
> introduced fast_io_fail_tmo but didn't export the value to sysfs.
> That
> means the value is hard coded during compile time. Export the timeout
> value to user space via sysfs to allow runtime configuration.
>
> Cc: Victor Gladkov
> Signed-off-by: Daniel Wagner
> ---
>
> This patch is against nvme-5.13
>
> BTW, checkpatch complains with
>
> WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not
> preferred. Consider using octal permissions '0644'.
>
> Is this something we want to adapt to?
>
> drivers/nvme/host/core.c | 31 +++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 40215a0246e4..c8de0e37c7d9 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3696,6 +3696,36 @@ static ssize_t
> nvme_ctrl_reconnect_delay_store(struct device *dev,
> static DEVICE_ATTR(reconnect_delay, S_IRUGO | S_IWUSR,
> nvme_ctrl_reconnect_delay_show,
> nvme_ctrl_reconnect_delay_store);
>
> +static ssize_t nvme_ctrl_fast_io_fail_tmo_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> + if (ctrl->opts->fast_io_fail_tmo == -1)
> + return sprintf(buf, "off\n");
> + return sprintf(buf, "%d\n", ctrl->opts->fast_io_fail_tmo);
> +}
> +
> +static ssize_t nvme_ctrl_fast_io_fail_tmo_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t
> count)
> +{
> + struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> + struct nvmf_ctrl_options *opts = ctrl->opts;
> + int fast_io_fail_tmo, err;
> +
> + err = kstrtoint(buf, 10, &fast_io_fail_tmo);
> + if (err)
> + return -EINVAL;
> +
> + else if (fast_io_fail_tmo < 0)
> + opts->fast_io_fail_tmo = -1;
> + else
> + opts->fast_io_fail_tmo = fast_io_fail_tmo;
> + return count;
> +}
> +static DEVICE_ATTR(fast_io_fail_tmo, S_IRUGO | S_IWUSR,
> + nvme_ctrl_fast_io_fail_tmo_show,
> nvme_ctrl_fast_io_fail_tmo_store);
> +
> static struct attribute *nvme_dev_attrs[] = {
> &dev_attr_reset_controller.attr,
> &dev_attr_rescan_controller.attr,
> @@ -3715,6 +3745,7 @@ static struct attribute *nvme_dev_attrs[] = {
> &dev_attr_hostid.attr,
> &dev_attr_ctrl_loss_tmo.attr,
> &dev_attr_reconnect_delay.attr,
> + &dev_attr_fast_io_fail_tmo.attr,
> NULL
> };
>
Thanks.
Reviewed-by: Ewan D. Milne
Re: [PATCH v2] nvme-tcp: Check if request has started before processing it
On Mon, 2021-03-15 at 10:16 -0700, Sagi Grimberg wrote: > > Hi Sagi, > > > > On Fri, Mar 05, 2021 at 11:57:30AM -0800, Sagi Grimberg wrote: > > > Daniel, again, there is nothing specific about this to nvme-tcp, > > > this is a safeguard against a funky controller (or a different > > > bug that is hidden by this). > > > > As far I can tell, the main difference between nvme-tcp and > > FC/NVMe, > > nvme-tcp has not a FW or a big driver which filter out some noise > > from a > > misbehaving controller. I haven't really checked the other > > transports > > but I wouldn't surprised they share the same properties as FC/NVMe. > > > > > The same can happen in any other transport so I would suggest > > > that if > > > this is a safeguard we want to put in place, we should make it a > > > generic one. > > > > > > i.e. nvme_tag_to_rq() that _all_ transports call consistently. > > > > Okay, I'll review all the relevant code and see what could made > > more > > generic and consistent. > > > > Though I think nvme-tcp plays in a different league as it is > > exposed to > > normal networking traffic and this is a very hostile environment. > > It is, but in this situation, the controller is sending a second > completion that results in a use-after-free, which makes the > transport irrelevant. Unless there is some other flow (which is > unclear > to me) that causes this which is a bug that needs to be fixed rather > than hidden with a safeguard. > The kernel should not crash regardless of any network traffic that is sent to the system. It should not be possible to either intentionally of mistakenly contruct packets that will deny service in this way. -Ewan
Re: md/dm-mpath: check whether all pgpaths have same uuid in multipath_ctr()
On Tue, 2021-03-23 at 15:47 +0800, lixiaokeng wrote:
>
> On 2021/3/22 22:22, Mike Snitzer wrote:
> > On Mon, Mar 22 2021 at 4:11am -0400,
> > Christoph Hellwig wrote:
> >
> > > On Sat, Mar 20, 2021 at 03:19:23PM +0800, Zhiqiang Liu wrote:
> > > > From: Zhiqiang Liu
> > > >
> > > > When we make IO stress test on multipath device, there will
> > > > be a metadata err because of wrong path. In the test, we
> > > > concurrent execute 'iscsi device login|logout' and
> > > > 'multipath -r' command with IO stress on multipath device.
> > > > In some case, systemd-udevd may have not time to process
> > > > uevents of iscsi device logout|login, and then 'multipath -r'
> > > > command triggers multipathd daemon calls ioctl to load table
> > > > with incorrect old device info from systemd-udevd.
> > > > Then, one iscsi path may be incorrectly attached to another
> > > > multipath which has different uuid. Finally, the metadata err
> > > > occurs when umounting filesystem to down write metadata on
> > > > the iscsi device which is actually not owned by the multipath
> > > > device.
> > > >
> > > > So we need to check whether all pgpaths of one multipath have
> > > > the same uuid, if not, we should throw a error.
> > > >
> > > > Signed-off-by: Zhiqiang Liu
> > > > Signed-off-by: lixiaokeng
> > > > Signed-off-by: linfeilong
> > > > Signed-off-by: Wubo
> > > > ---
> > > > drivers/md/dm-mpath.c | 52
> > > > +
> > > > drivers/scsi/scsi_lib.c | 1 +
> > > > 2 files changed, 53 insertions(+)
> > > >
> > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > > > index bced42f082b0..f0b995784b53 100644
> > > > --- a/drivers/md/dm-mpath.c
> > > > +++ b/drivers/md/dm-mpath.c
> > > > @@ -24,6 +24,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > >
> > > > @@ -1169,6 +1170,45 @@ static int parse_features(struct
> > > > dm_arg_set *as, struct multipath *m)
> > > > return r;
> > > > }
> > > >
> > > > +#define SCSI_VPD_LUN_ID_PREFIX_LEN 4
> > > > +#define MPATH_UUID_PREFIX_LEN 7
> > > > +static int check_pg_uuid(struct priority_group *pg, char
> > > > *md_uuid)
> > > > +{
> > > > + char pgpath_uuid[DM_UUID_LEN] = {0};
> > > > + struct request_queue *q;
> > > > + struct pgpath *pgpath;
> > > > + struct scsi_device *sdev;
> > > > + ssize_t count;
> > > > + int r = 0;
> > > > +
> > > > + list_for_each_entry(pgpath, &pg->pgpaths, list) {
> > > > + q = bdev_get_queue(pgpath->path.dev->bdev);
> > > > + sdev = scsi_device_from_queue(q);
> > >
> > > Common dm-multipath code should never poke into scsi
> > > internals. This
> > > is something for the device handler to check. It probably also
> > > won't
> > > work for all older devices.
> >
> > Definitely.
> >
> > But that aside, userspace (multipathd) _should_ be able to do extra
> > validation, _before_ pushing down a new table to the kernel, rather
> > than
> > forcing the kernel to do it.
> >
>
> Martin (committer of multipath-tools) said that:
> "Don't get me wrong, I don't argue against tough testing. But we
> should
> be aware that there are always time intervals during which
> multipathd's
> picture of the present devices is different from what the kernel
> sees."
>
> It is difficult to solve this in multipathd.
>
> Regards,
> Lixiaokeng
>
I think the patch is no good. There are plenty of devices that don't
support VPD page 83h:
int scsi_vpd_lun_id(struct scsi_device *sdev, char *id, size_t id_len)
{
u8 cur_id_type = 0xff;
u8 cur_id_size = 0;
unsigned char *d, *cur_id_str;
unsigned char __rcu *vpd_pg83;
int id_size = -EINVAL;
rcu_read_lock();
vpd_pg83 = rcu_dereference(sdev->vpd_pg83);
if (!vpd_pg83) {
rcu_read_unlock();
return -ENXIO;
}
and the DM layer should not be looking at the properties of the
underlying devices in this way anyway. It should be pushed down
to the table.
-Ewan
Re: [PATCH] scsi_logging: print cdb into new line after opcode
On Fri, 2021-01-22 at 09:39 +0100, Martin Kepplinger wrote:
> The current log message results in a line like the following where
> the first byte is duplicated, giving a wrong impression:
>
> sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 60 40 00 00 01
> 00
>
> Print the cdb into a new line in any case, not only when cmd_len is
> greater than 16. The above example error will then read:
>
> sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28
> 28 00 01 c0 09 00 00 00 08 00
>
> Signed-off-by: Martin Kepplinger
> ---
> drivers/scsi/scsi_logging.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/scsi_logging.c
> b/drivers/scsi/scsi_logging.c
> index 8ea44c6595ef..0081d3936f83 100644
> --- a/drivers/scsi/scsi_logging.c
> +++ b/drivers/scsi/scsi_logging.c
> @@ -200,10 +200,11 @@ void scsi_print_command(struct scsi_cmnd *cmd)
> if (off >= logbuf_len)
> goto out_printk;
>
> + /* Print opcode in one line and use separate lines for CDB */
> + off += scnprintf(logbuf + off, logbuf_len - off, "\n");
> +
> /* print out all bytes in cdb */
> if (cmd->cmd_len > 16) {
> - /* Print opcode in one line and use separate lines for
> CDB */
> - off += scnprintf(logbuf + off, logbuf_len - off, "\n");
> dev_printk(KERN_INFO, &cmd->device->sdev_gendev, "%s",
> logbuf);
> for (k = 0; k < cmd->cmd_len; k += 16) {
> size_t linelen = min(cmd->cmd_len - k, 16);
> @@ -224,7 +225,6 @@ void scsi_print_command(struct scsi_cmnd *cmd)
> goto out;
> }
> if (!WARN_ON(off > logbuf_len - 49)) {
> - off += scnprintf(logbuf + off, logbuf_len - off, " ");
> hex_dump_to_buffer(cmd->cmnd, cmd->cmd_len, 16, 1,
> logbuf + off, logbuf_len - off,
> false);
I'd rather we not change this.
-Ewan
Re: [PATCH][next] scsi: pm8001: remove space in a debug message
On Tue, 2020-11-24 at 09:38 +, Colin King wrote: > From: Colin Ian King > > There are two words that need separating with a space in a > pm8001_dbg message. Fix it. > > Signed-off-by: Colin Ian King > --- > drivers/scsi/pm8001/pm8001_hwi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/pm8001/pm8001_hwi.c > b/drivers/scsi/pm8001/pm8001_hwi.c > index 08d6cc9b50db..c8d4d87c5473 100644 > --- a/drivers/scsi/pm8001/pm8001_hwi.c > +++ b/drivers/scsi/pm8001/pm8001_hwi.c > @@ -1031,7 +1031,7 @@ pm8001_chip_soft_rst(struct pm8001_hba_info > *pm8001_ha) > regVal = pm8001_cr32(pm8001_ha, 2, > GSM_WRITE_DATA_PARITY_CHECK); > pm8001_cw32(pm8001_ha, 2, GSM_WRITE_DATA_PARITY_CHECK, > regVal3); > pm8001_dbg(pm8001_ha, INIT, > -"GSM 0x700048 - Write Data Parity Check Enableis set > to = 0x%x\n", > +"GSM 0x700048 - Write Data Parity Check Enable is > set to = 0x%x\n", > pm8001_cr32(pm8001_ha, 2, > GSM_WRITE_DATA_PARITY_CHECK)); > > /* step 13: bring the IOP and AAP1 out of reset */ Reviewed-by: Ewan D. Milne
Re: [PATCH v2] scsi: fcoe: add missed kfree() in an error path
See below.
On Thu, 2020-07-09 at 20:05 +0800, Jing Xiangfeng wrote:
> fcoe_fdmi_info() misses to call kfree() in an error path.
> Add a label 'free_fdmi' and jump to it.
>
> Fixes: f07d46bbc9ba ("fcoe: Fix smatch warning in fcoe_fdmi_info
> function")
> Signed-off-by: Jing Xiangfeng
> ---
> drivers/scsi/fcoe/fcoe.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 25dae9f0b205..a63057a03772 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -830,7 +830,7 @@ static void fcoe_fdmi_info(struct fc_lport
> *lport, struct net_device *netdev)
> if (rc) {
> printk(KERN_INFO "fcoe: Failed to retrieve FDMI
> "
> "information from netdev.\n");
> - return;
> + goto free_fdmi;
> }
>
> snprintf(fc_host_serial_number(lport->host),
> @@ -868,6 +868,7 @@ static void fcoe_fdmi_info(struct fc_lport
> *lport, struct net_device *netdev)
>
> /* Enable FDMI lport states */
> lport->fdmi_enabled = 1;
> +free_fdmi:
> kfree(fdmi);
> } else {
> lport->fdmi_enabled = 0;
Normally I would like to see goto labels for error paths outside
conditionals and at the end of the function. In this case it would
seem to be cleaner to put an else { } clause in the if (rc) above
around the snprintf() calls.
-Ewan
Re: [PATCH] scsi: associate bio write hint with WRITE CDB
On Thu, 2019-01-03 at 16:00 -0500, Douglas Gilbert wrote:
> On 2019-01-03 4:47 a.m., Randall Huang wrote:
> > On Wed, Jan 02, 2019 at 11:51:33PM -0800, Christoph Hellwig wrote:
> > > On Wed, Dec 26, 2018 at 12:15:04PM +0800, Randall Huang wrote:
> > > > In SPC-3, WRITE(10)/(16) support grouping function.
> > > > Let's associate bio write hint with group number for
> > > > enabling StreamID or Turbo Write feature.
> > > >
> > > > Signed-off-by: Randall Huang
> > > > ---
> > > > drivers/scsi/sd.c | 14 --
> > > > 1 file changed, 12 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > > > index 4b49cb67617e..28bfa9ed2b54 100644
> > > > --- a/drivers/scsi/sd.c
> > > > +++ b/drivers/scsi/sd.c
> > > > @@ -1201,7 +1201,12 @@ static int sd_setup_read_write_cmnd(struct
> > > > scsi_cmnd *SCpnt)
> > > > SCpnt->cmnd[11] = (unsigned char) (this_count >> 16) &
> > > > 0xff;
> > > > SCpnt->cmnd[12] = (unsigned char) (this_count >> 8) &
> > > > 0xff;
> > > > SCpnt->cmnd[13] = (unsigned char) this_count & 0xff;
> > > > - SCpnt->cmnd[14] = SCpnt->cmnd[15] = 0;
> > > > + if (rq_data_dir(rq) == WRITE) {
> > > > + SCpnt->cmnd[14] = rq->bio->bi_write_hint & 0x3f;
> > > > + } else {
> > > > + SCpnt->cmnd[14] = 0;
> > > > + }
> > >
> > > No need for braces here.
> >
> > Already send a new version
> > >
> > > But what I'm more worried about is devices not recognizing the feature
> > > throwing up on the field. Can you check what SBC version first
> > > references these or come up with some other decently smart conditional?
> >
> > My reference is SCSI Block Commands – 3 (SBC-3) Revision 25.
> > Section 5.32 WRITE (10) and 5.34 WRITE (16)
> >
> > > Maybe Martin has a good idea, too.
>
> That is the GROUP NUMBER field. Also found in READ(16) at the same
> location within its cdb. The proposed code deserves at least an
> explanatory comment.
>
> Since it is relatively recent, perhaps the above should only be done iff:
> - the REPORT SUPPORTED OPERATION CODES (RSOC) command is supported, and
> - in the RSOC entry for WRITE(16), the CDB USAGE DATA field (a bit mask)
> indicates the GROUP NUMBER field is supported
>
> That check can be done once, at disk attachment time where there is already
> code to fetch RSOC.
>
>
> Is there a bi_read_hint ? If not then the bi_write_hint should also be applied
> to READ(16). Makes that variable naming look pretty silly though.
>
> Doug Gilbert
>
SBC-5 says that support for the grouping function is indicated by the
GROUP_SUP bit in the Extended Inquiry VPD page (86h). I'm not sure how
many devices actually support that page though. Probably most don't.
What devices actually DO support the grouping and do something with it?
We'd probably need a blacklist flag to turn this off and/or some code in
the error path to discontinue setting the field if the device returns
INVALID FIELD IN CDB or something, like we do for disabling discard
commands if they don't actually work in sd_done().
-Ewan
Re: [PATCH] scsi: libfc: fix incorrect variable assingment
On Thu, 2017-05-11 at 17:24 -0500, Gustavo A. R. Silva wrote:
> Previous assignment was causing the use of the uninitialized variable
> _explan_ inside fc_seq_ls_rjt() function, which in this particular
> case is being called by fc_seq_els_rsp_send().
>
> Addresses-Coverity-ID: 1398125
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/scsi/libfc/fc_rport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index b44c313..5203258 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -1422,7 +1422,7 @@ static void fc_rport_recv_rtv_req(struct fc_rport_priv
> *rdata,
> fp = fc_frame_alloc(lport, sizeof(*rtv));
> if (!fp) {
> rjt_data.reason = ELS_RJT_UNAB;
> - rjt_data.reason = ELS_EXPL_INSUF_RES;
> + rjt_data.explan = ELS_EXPL_INSUF_RES;
> fc_seq_els_rsp_send(in_fp, ELS_LS_RJT, &rjt_data);
> goto drop;
> }
s/assingment/assignment/
Reviewed-by: Ewan D. Milne
Re: [PATCH v4] sd: Ignore sync cache failures when not supported
/* ignore OFFLINE device */ > if (ret == -ENODEV) > - ret = 0; > - goto done; > + return 0; > + > + if (!scsi_sense_valid(&sshdr) || > + sshdr.sense_key != ILLEGAL_REQUEST) > + return ret; > + > + /* > + * sshdr.sense_key == ILLEGAL_REQUEST means this drive > + * doesn't support sync. There's not much to do and > + * suspend shouldn't fail. > + */ > + ret = 0; > } > } > > @@ -3359,7 +3376,6 @@ static int sd_suspend_common(struct device *dev, bool > ignore_stop_errors) > ret = 0; > } > > -done: > return ret; > } > Reviewed-by: Ewan D. Milne
Re: [PATCH v3] sd: Ignore sync cache failures when not supported
On Tue, 2017-05-09 at 17:43 +0200, Thierry Escande wrote:
> From: Derek Basehore
>
> Some external hard drives don't support the sync command even though the
> hard drive has write cache enabled. In this case, upon suspend request,
> sync cache failures are ignored if the error code in the sense header is
> ILLEGAL_REQUEST. There's not much we can do for these drives, so we
> shouldn't fail to suspend for this error case. The drive may stay
> powered if that's the setup for the port it's plugged into.
>
> Signed-off-by: Derek Basehore
> Signed-off-by: Thierry Escande
> ---
>
> v3 changes:
> - Pass the sense_hdr structure to sd_sync_cache() instead of the
> lonely sense_key field
>
> v2 changes:
> - Change sense_key type to u8 in sd_sync_cache()
>
> drivers/scsi/sd.c | 34 +-
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fcfeddc..ee69fee 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1489,17 +1489,21 @@ static unsigned int sd_check_events(struct gendisk
> *disk, unsigned int clearing)
> return retval;
> }
>
> -static int sd_sync_cache(struct scsi_disk *sdkp)
> +static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr
> *sshdr)
> {
> int retries, res;
> struct scsi_device *sdp = sdkp->device;
> const int timeout = sdp->request_queue->rq_timeout
> * SD_FLUSH_TIMEOUT_MULTIPLIER;
> - struct scsi_sense_hdr sshdr;
> + struct scsi_sense_hdr my_sshdr;
>
> if (!scsi_device_online(sdp))
> return -ENODEV;
>
> + /* caller might not be interested in sense, but we need it */
> + if (!sshdr)
> + sshdr = &my_sshdr;
> +
> for (retries = 3; retries > 0; --retries) {
> unsigned char cmd[10] = { 0 };
>
> @@ -1508,7 +1512,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>* Leave the rest of the command zero to indicate
>* flush everything.
>*/
> - res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
> + res = scsi_execute(sdp, cmd, DMA_NONE, NULL, 0, NULL, sshdr,
> timeout, SD_MAX_RETRIES, 0, RQF_PM, NULL);
> if (res == 0)
> break;
> @@ -1518,11 +1522,12 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> sd_print_result(sdkp, "Synchronize Cache(10) failed", res);
>
> if (driver_byte(res) & DRIVER_SENSE)
> - sd_print_sense_hdr(sdkp, &sshdr);
> + sd_print_sense_hdr(sdkp, sshdr);
> +
> /* we need to evaluate the error return */
> - if (scsi_sense_valid(&sshdr) &&
> - (sshdr.asc == 0x3a || /* medium not present */
> - sshdr.asc == 0x20))/* invalid command */
> + if (scsi_sense_valid(sshdr) &&
> + (sshdr->asc == 0x3a || /* medium not present */
> + sshdr->asc == 0x20)) /* invalid command */
> /* this is no error here */
> return 0;
>
> @@ -3323,7 +3328,7 @@ static void sd_shutdown(struct device *dev)
>
> if (sdkp->WCE && sdkp->media_present) {
> sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> - sd_sync_cache(sdkp);
> + sd_sync_cache(sdkp, NULL);
> }
>
> if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> @@ -3335,6 +3340,7 @@ static void sd_shutdown(struct device *dev)
> static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
> {
> struct scsi_disk *sdkp = dev_get_drvdata(dev);
> + struct scsi_sense_hdr sshdr = { .sense_key = NO_SENSE };
> int ret = 0;
>
> if (!sdkp) /* E.g.: runtime suspend following sd_remove() */
> @@ -3342,8 +3348,17 @@ static int sd_suspend_common(struct device *dev, bool
> ignore_stop_errors)
>
> if (sdkp->WCE && sdkp->media_present) {
> sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> - ret = sd_sync_cache(sdkp);
> + ret = sd_sync_cache(sdkp, &sshdr);
> if (ret) {
> + /*
> + * If this drive doesn't support sync, there's not much
> + * to do and suspend shouldn't fail.
> + */
> + if (sshdr.sense_key == ILLEGAL_REQUEST) {
> + ret = 0;
> + goto start_stop;
> + }
> +
> /* ignore OFFLINE device */
> if (ret == -ENODEV)
> ret = 0;
I think you need to check for scsi_sense_valid(sshdr) prior to
examining the .sense_key. Also, I believe the check for -ENODEV
should be made prior to chec
Re: [PATCH] scsi: qedi: select UIO
On Tue, 2017-01-10 at 16:27 +0100, Arnd Bergmann wrote:
> The newly added qedi driver links against the UIO framework, but can
> be built without that:
>
> drivers/scsi/qedi/qedi_main.o: In function `qedi_free_uio':
> qedi_main.c:(.text.qedi_free_uio+0x78): undefined reference to
> `uio_unregister_device'
> drivers/scsi/qedi/qedi_main.o: In function `qedi_ll2_recv_thread':
> qedi_main.c:(.text.qedi_ll2_recv_thread+0x18c): undefined reference to
> `uio_event_notify'
> drivers/scsi/qedi/qedi_main.o: In function `__qedi_probe.constprop.1':
> qedi_main.c:(.text.__qedi_probe.constprop.1+0x1368): undefined reference to
> `__uio_register_device'
>
> This adds a compile-time dependency.
>
> Fixes: ace7f46ba5fd ("scsi: qedi: Add QLogic FastLinQ offload iSCSI driver
> framework.")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/scsi/qedi/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/qedi/Kconfig b/drivers/scsi/qedi/Kconfig
> index 23ca8a274586..913610f3d274 100644
> --- a/drivers/scsi/qedi/Kconfig
> +++ b/drivers/scsi/qedi/Kconfig
> @@ -2,6 +2,7 @@ config QEDI
> tristate "QLogic QEDI 25/40/100Gb iSCSI Initiator Driver Support"
> depends on PCI && SCSI
> depends on QED
> + depends on UIO
> select SCSI_ISCSI_ATTRS
> select QED_LL2
> select QED_ISCSI
Randy posted a similar patch back in December but I don't think there
was ever a reply to Christoph's question about why qedi depends on uio.
-Ewan
Re: [PATCH] snic: Return error code on memory allocation failure
On Wed, 2016-12-21 at 14:45 +0100, Burak Ok wrote:
> If a call to mempool_create_slab_pool() in snic_probe() returns NULL,
> return -ENOMEM to indicate failure. mempool_creat_slab_pool() only fails if
> it cannot allocate memory.
>
> https://bugzilla.kernel.org/show_bug.cgi?id=189061
>
> Reported-by: bianpan2...@ruc.edu.cn
> Signed-off-by: Burak Ok
> Signed-off-by: Andreas Schaertl
> ---
> drivers/scsi/snic/snic_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/snic/snic_main.c b/drivers/scsi/snic/snic_main.c
> index 396b32d..7cf70aa 100644
> --- a/drivers/scsi/snic/snic_main.c
> +++ b/drivers/scsi/snic/snic_main.c
> @@ -591,6 +591,7 @@ snic_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> if (!pool) {
> SNIC_HOST_ERR(shost, "dflt sgl pool creation failed\n");
>
> + ret = -ENOMEM;
> goto err_free_res;
> }
>
> @@ -601,6 +602,7 @@ snic_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> if (!pool) {
> SNIC_HOST_ERR(shost, "max sgl pool creation failed\n");
>
> + ret = -ENOMEM;
> goto err_free_dflt_sgl_pool;
> }
>
> @@ -611,6 +613,7 @@ snic_probe(struct pci_dev *pdev, const struct
> pci_device_id *ent)
> if (!pool) {
> SNIC_HOST_ERR(shost, "snic tmreq info pool creation failed.\n");
>
> + ret = -ENOMEM;
> goto err_free_max_sgl_pool;
> }
>
Reviewed-by: Ewan D. Milne
Re: [RESEND PATCH v2 2/2] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
On Wed, 2016-03-30 at 09:09 +0200, Johannes Thumshirn wrote:
> Add intermediate STARGET_REMOVE state to scsi_target_state to avoid running
> into the BUG_ON() in scsi_target_reap().
>
> This intermediate state is only valid in the path from scsi_remove_target() to
> scsi_target_destroy() indicating this target is going to be removed.
>
> Signed-off-by: Johannes Thumshirn
> Fixes: 40998193560dab6c3ce8d25f4fa58a23e252ef38
> Cc: sta...@vger.kernel.org
> Reviewed-by: Hannes Reinecke
> Reviewed-by: Ewan D. Milne
> ---
>
> Changes from v1:
> * The state transition from STARGET_CREATED to STARGET_DEL is legitimate,
> so don't BUG() on it. Found by the 0-Day Bot.
>
>
> drivers/scsi/scsi_scan.c | 2 ++
> drivers/scsi/scsi_sysfs.c | 4 +++-
> include/scsi/scsi_device.h | 1 +
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6a82066..63b8bca 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -315,6 +315,8 @@ static void scsi_target_destroy(struct scsi_target
> *starget)
> struct Scsi_Host *shost = dev_to_shost(dev->parent);
> unsigned long flags;
>
> + BUG_ON(starget->state != STARGET_REMOVE &&
> +starget->state != STARGET_CREATED);
> starget->state = STARGET_DEL;
> transport_destroy_device(dev);
> spin_lock_irqsave(shost->host_lock, flags);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 4f18a85..9e5f893 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1278,10 +1278,12 @@ void scsi_remove_target(struct device *dev)
> restart:
> spin_lock_irqsave(shost->host_lock, flags);
> list_for_each_entry(starget, &shost->__targets, siblings) {
> - if (starget->state == STARGET_DEL)
> + if (starget->state == STARGET_DEL ||
> + starget->state == STARGET_REMOVE)
> continue;
> if (starget->dev.parent == dev || &starget->dev == dev) {
> kref_get(&starget->reap_ref);
> + starget->state = STARGET_REMOVE;
> spin_unlock_irqrestore(shost->host_lock, flags);
> __scsi_remove_target(starget);
> scsi_target_reap(starget);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index f63a167..2bffaa6 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -240,6 +240,7 @@ scmd_printk(const char *, const struct scsi_cmnd *, const
> char *, ...);
> enum scsi_target_state {
> STARGET_CREATED = 1,
> STARGET_RUNNING,
> + STARGET_REMOVE,
> STARGET_DEL,
> };
>
Reviewed-by: Ewan D. Milne
Re: [RESEND PATCH v2 1/2] Revert "scsi: fix soft lockup in scsi_remove_target() on module removal"
On Wed, 2016-03-30 at 09:09 +0200, Johannes Thumshirn wrote:
> This reverts commit 90a88d6ef88edcfc4f644dddc7eef4ea41bccf8b.
>
> Signed-off-by: Johannes Thumshirn
> Cc: sta...@vger.kernel.org
> ---
> drivers/scsi/scsi_sysfs.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 00bc721..4f18a85 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1272,18 +1272,16 @@ static void __scsi_remove_target(struct scsi_target
> *starget)
> void scsi_remove_target(struct device *dev)
> {
> struct Scsi_Host *shost = dev_to_shost(dev->parent);
> - struct scsi_target *starget, *last_target = NULL;
> + struct scsi_target *starget;
> unsigned long flags;
>
> restart:
> spin_lock_irqsave(shost->host_lock, flags);
> list_for_each_entry(starget, &shost->__targets, siblings) {
> - if (starget->state == STARGET_DEL ||
> - starget == last_target)
> + if (starget->state == STARGET_DEL)
> continue;
> if (starget->dev.parent == dev || &starget->dev == dev) {
> kref_get(&starget->reap_ref);
> - last_target = starget;
> spin_unlock_irqrestore(shost->host_lock, flags);
> __scsi_remove_target(starget);
> scsi_target_reap(starget);
Reviewed-by: Ewan D. Milne
Re: [PATCH] scsi: Add intermediate STARGET_REMOVE state to scsi_target_state
On Thu, 2016-03-24 at 10:56 +0100, Johannes Thumshirn wrote:
> The target state machine only knows 'STARGET_DEL', which is set once
> scsi_target_destroy() is called.
> However, by that time the structure is still part of the __stargets
> list of the SCSI host, so any concurrent invocation will see this as
> a valid target, causing an access to freed memory.
>
> This patch adds an intermediate state 'STARGET_REMOVE', which is set
> as soon as the target is scheduled to be removed.
> With this any concurrent invocation will be able to skip these
> targets and avoid the above scenario.
>
> Signed-off-by: Johannes Thumshirn
> Fixes: 90a88d6ef 'scsi: fix soft lockup in scsi_remove_target() on module
> removal'
> Cc: sta...@vger.kernel.org
> Reviewed-by: Hannes Reinecke
> ---
> drivers/scsi/scsi_scan.c | 1 +
> drivers/scsi/scsi_sysfs.c | 2 ++
> include/scsi/scsi_device.h | 1 +
> 3 files changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6a82066..37459dfa 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -315,6 +315,7 @@ static void scsi_target_destroy(struct scsi_target
> *starget)
> struct Scsi_Host *shost = dev_to_shost(dev->parent);
> unsigned long flags;
>
> + BUG_ON(starget->state != STARGET_REMOVE);
> starget->state = STARGET_DEL;
> transport_destroy_device(dev);
> spin_lock_irqsave(shost->host_lock, flags);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 00bc721..0df82e8 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1279,11 +1279,13 @@ restart:
> spin_lock_irqsave(shost->host_lock, flags);
> list_for_each_entry(starget, &shost->__targets, siblings) {
> if (starget->state == STARGET_DEL ||
> + starget->state == STARGET_REMOVE ||
> starget == last_target)
> continue;
> if (starget->dev.parent == dev || &starget->dev == dev) {
> kref_get(&starget->reap_ref);
> last_target = starget;
> + starget->state = STARGET_REMOVE;
> spin_unlock_irqrestore(shost->host_lock, flags);
> __scsi_remove_target(starget);
> scsi_target_reap(starget);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index f63a167..2bffaa6 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -240,6 +240,7 @@ scmd_printk(const char *, const struct scsi_cmnd *, const
> char *, ...);
> enum scsi_target_state {
> STARGET_CREATED = 1,
> STARGET_RUNNING,
> + STARGET_REMOVE,
> STARGET_DEL,
> };
>
This looks fine. Do we still need 90a88d6ef (scsi: fix soft lockup in
scsi_remove_target() on module removal) or can that be reverted now,
since the STARGET_REMOVE state will allow the iteration to continue?
Reviewed-by: Ewan D. Milne
Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning
On Wed, 2016-03-16 at 16:03 +0100, Tomas Henzl wrote:
> On 15.3.2016 22:40, Arnd Bergmann wrote:
> > The qlt_check_reserve_free_req() function produces an incorrect warning
> > when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
> >
> > drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
> > drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used
> > uninitialized in this function [-Werror=maybe-uninitialized]
> >ql_dbg(ql_dbg_io, vha, 0x305a,
> >^~
> >"qla_target(%d): There is no room in the request ring:
> > vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d
> > Req-Length=%d\n",
> >
> > ~~~
> >vha->vp_idx, vha->req->ring_index,
> >~~
> >vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> >~~
> > drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used
> > uninitialized in this function [-Werror=maybe-uninitialized]
> >
> > The problem is that gcc fails to track the state of the condition across
> > an annotated branch.
> >
> > This slightly rearranges the code to move the second if() block
> > into the first one, to avoid the warning while retaining the
> > behavior of the code.
>
> When the first 'if' is true the vha->req->ring_index gets a new value
> assigned - so it could be possible that the second 'if' wont be true any more.
> The code should not be merged into that single 'if', or am I missing
> something?
>
> tomash
If the first "if" is false, the second "if" will be false also, because
the vha->req->cnt value has not changed. If the first "if" is true, the
nested second "if" will retest the condition.
The compiler is not at fault, because vha->req->cnt can't be tracked as
it could be modified by another thread/process. It isn't, it's protected
by the ->hardware_lock, but the compiler doesn't know that.
-Ewan
> >
> > Signed-off-by: Arnd Bergmann
> > ---
> > drivers/scsi/qla2xxx/qla_target.c | 16 +---
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_target.c
> > b/drivers/scsi/qla2xxx/qla_target.c
> > index 985231900aca..8a44d1541eb4 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct
> > scsi_qla_host *vha,
> > else
> > vha->req->cnt = vha->req->length -
> > (vha->req->ring_index - cnt);
> > - }
> >
> > - if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> > - ql_dbg(ql_dbg_io, vha, 0x305a,
> > - "qla_target(%d): There is no room in the request ring:
> > vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d
> > Req-Length=%d\n",
> > - vha->vp_idx, vha->req->ring_index,
> > - vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> > - return -EAGAIN;
> > + if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> > + ql_dbg(ql_dbg_io, vha, 0x305a,
> > + "qla_target(%d): There is no room in the request
> > ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d
> > Req-in=%d Req-Length=%d\n",
> > + vha->vp_idx, vha->req->ring_index,
> > + vha->req->cnt, req_cnt, cnt, cnt_in,
> > + vha->req->length);
> > + return -EAGAIN;
> > + }
> > }
> > +
> > vha->req->cnt -= req_cnt;
> >
> > return 0;
>
Re: [PATCH] scsi: fc: use get/put_unaligned64 for wwn access
On Wed, 2016-03-16 at 17:39 +0100, Arnd Bergmann wrote:
> A bug in the gcc-6.0 prerelease version caused at least one
> driver (lpfc) to have excessive stack usage when dealing with
> wwn data, on the ARM architecture.
>
> lpfc_scsi.c: In function 'lpfc_find_next_oas_lun':
> lpfc_scsi.c:117:1: warning: the frame size of 1152 bytes is larger than 1024
> bytes [-Wframe-larger-than=]
>
> I have reported this as a gcc regression in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70232
>
> However, using a better implementation of wwn_to_u64() not only
> helps with the particular gcc problem but also leads to better
> object code for any version or architecture.
>
> The kernel already provides get_unaligned_be64() and
> put_unaligned_be64() helper functions that provide an
> optimized implementation with the desired semantics.
>
> The lpfc_find_next_oas_lun() function in the example that
> grew from 1146 bytes to 5144 bytes when moving from gcc-5.3
> to gcc-6.0 is now 804 bytes, as the optimized
> get_unaligned_be64() load can be done in three instructions.
> The stack usage is now down to 28 bytes from 128 bytes with
> gcc-5.3 before.
>
> Signed-off-by: Arnd Bergmann
> ---
> include/scsi/scsi_transport_fc.h | 15 +++
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/include/scsi/scsi_transport_fc.h
> b/include/scsi/scsi_transport_fc.h
> index 784bc2c0929f..bf66ea6bed2b 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -28,6 +28,7 @@
> #define SCSI_TRANSPORT_FC_H
>
> #include
> +#include
> #include
> #include
>
> @@ -797,22 +798,12 @@ fc_remote_port_chkready(struct fc_rport *rport)
>
> static inline u64 wwn_to_u64(u8 *wwn)
> {
> - return (u64)wwn[0] << 56 | (u64)wwn[1] << 48 |
> - (u64)wwn[2] << 40 | (u64)wwn[3] << 32 |
> - (u64)wwn[4] << 24 | (u64)wwn[5] << 16 |
> - (u64)wwn[6] << 8 | (u64)wwn[7];
> + return get_unaligned_be64(wwn);
> }
>
> static inline void u64_to_wwn(u64 inm, u8 *wwn)
> {
> - wwn[0] = (inm >> 56) & 0xff;
> - wwn[1] = (inm >> 48) & 0xff;
> - wwn[2] = (inm >> 40) & 0xff;
> - wwn[3] = (inm >> 32) & 0xff;
> - wwn[4] = (inm >> 24) & 0xff;
> - wwn[5] = (inm >> 16) & 0xff;
> - wwn[6] = (inm >> 8) & 0xff;
> - wwn[7] = inm & 0xff;
> + put_unaligned_be64(inm, wwn);
> }
>
> /**
It would be nice to get rid of these functions completely and just
change the callers to use get/put_unaligned_be64() directly, like libfc
does, but that involves changing 7 drivers and scsi_transport_fc.
Reviewed-by: Ewan D. Milne
[PATCH v3] mptbase: fixup error handling paths in mpt_attach()
From: "Ewan D. Milne"
mpt_attach() was not checking for the failure to create fw_event_q.
Also, iounmap() was not being called in all error cases after ioremap()
had been called by mpt_mapresources().
Reported-by: Insu Yun
Reviewed-by: Tomas Henzl
Signed-off-by: Ewan D. Milne
---
drivers/message/fusion/mptbase.c | 40 +---
1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5dcc031..d3b130a 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1801,8 +1801,7 @@ mpt_attach(struct pci_dev *pdev, const struct
pci_device_id *id)
ioc->pcidev = pdev;
if (mpt_mapresources(ioc)) {
- kfree(ioc);
- return r;
+ goto out_free_ioc;
}
/*
@@ -1871,9 +1870,8 @@ mpt_attach(struct pci_dev *pdev, const struct
pci_device_id *id)
if (!ioc->reset_work_q) {
printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
ioc->name);
- pci_release_selected_regions(pdev, ioc->bars);
- kfree(ioc);
- return -ENOMEM;
+ r = -ENOMEM;
+ goto out_unmap_resources;
}
dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
@@ -1995,16 +1993,27 @@ mpt_attach(struct pci_dev *pdev, const struct
pci_device_id *id)
spin_lock_init(&ioc->fw_event_lock);
snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id);
ioc->fw_event_q = create_singlethread_workqueue(ioc->fw_event_q_name);
+ if (!ioc->fw_event_q) {
+ printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
+ ioc->name);
+ r = -ENOMEM;
+ goto out_remove_ioc;
+ }
if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
CAN_SLEEP)) != 0){
printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
ioc->name, r);
+ destroy_workqueue(ioc->fw_event_q);
+ ioc->fw_event_q = NULL;
+
list_del(&ioc->list);
if (ioc->alt_ioc)
ioc->alt_ioc->alt_ioc = NULL;
iounmap(ioc->memmap);
+ if (pci_is_enabled(pdev))
+ pci_disable_device(pdev);
if (r != -5)
pci_release_selected_regions(pdev, ioc->bars);
@@ -2012,7 +2021,6 @@ mpt_attach(struct pci_dev *pdev, const struct
pci_device_id *id)
ioc->reset_work_q = NULL;
kfree(ioc);
- pci_set_drvdata(pdev, NULL);
return r;
}
@@ -2040,6 +2048,24 @@ mpt_attach(struct pci_dev *pdev, const struct
pci_device_id *id)
msecs_to_jiffies(MPT_POLLING_INTERVAL));
return 0;
+
+out_remove_ioc:
+ list_del(&ioc->list);
+ if (ioc->alt_ioc)
+ ioc->alt_ioc->alt_ioc = NULL;
+
+ destroy_workqueue(ioc->reset_work_q);
+ ioc->reset_work_q = NULL;
+
+out_unmap_resources:
+ iounmap(ioc->memmap);
+ pci_disable_device(pdev);
+ pci_release_selected_regions(pdev, ioc->bars);
+
+out_free_ioc:
+ kfree(ioc);
+
+ return r;
}
/*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -6229,7 +6255,7 @@ mpt_get_manufacturing_pg_0(MPT_ADAPTER *ioc)
memcpy(ioc->board_assembly, pbuf->BoardAssembly,
sizeof(ioc->board_assembly));
memcpy(ioc->board_tracer, pbuf->BoardTracerNumber,
sizeof(ioc->board_tracer));
- out:
+out:
if (pbuf)
pci_free_consistent(ioc->pcidev, hdr.PageLength * 4, pbuf,
buf_dma);
--
2.1.0
