linux-kernel - Re: [PATCH 2/2] mvsas: add support for Task collector mode and fixed relative bugs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTi=xZnK6--p0gp+MAH1aRRJsPw9=Lg@mail.gmail.com>
Date:	Wed, 25 May 2011 13:41:47 -0700
From:	Dan Williams <dan.j.williams@...el.com>
To:	yxlraid@...il.com
Cc:	James.Bottomley@...e.de, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, yuxiangl@...vell.com,
	jfeng@...vell.com
Subject: Re: [PATCH 2/2] mvsas: add support for Task collector mode and fixed
 relative bugs

On Tue, Apr 26, 2011 at 6:36 AM,  <yxlraid@...il.com> wrote:
> From: Xiangliang Yu <yuxiangl@...vell.com>
>
> 1.Add support for Task collector mode.
> 2.Fixed relative collector mode bug:
> - I/O failed when disks is on two ports
> - system hang when hotplug disk
> - system hang when unplug disk during run IO
> 3.Unlock ap->lock within .lldd_execute_task for direct mode to improve performance

Can you share more details about the performance impact?  Seems like
this should be made generic across all libsas drivers... see below.

[..]
> +static int mvs_task_exec(struct sas_task *task, const int num, gfp_t gfp_flags,
> +                               struct completion *completion, int is_tmf,
> +                               struct mvs_tmf_task *tmf)
> +{
> +       struct domain_device *dev = task->dev;
> +       struct mvs_info *mvi = NULL;
> +       u32 rc = 0;
> +       u32 pass = 0;
> +       unsigned long flags = 0;
> +
> +       mvi = ((struct mvs_device *)task->dev->lldd_dev)->mvi_info;
> +
> +       if ((dev->dev_type == SATA_DEV) && (dev->sata_dev.ap != NULL))
> +               spin_unlock_irq(dev->sata_dev.ap->lock);
> +
> +       spin_lock_irqsave(&mvi->lock, flags);

This is fine in current code, but isn't it dangerous to assume that
libata is the only agent that might have interrupts disabled?
Especially confusing since this code turns around and uses
spin_lock_irqsave() as the very next step, which leaves a casual
reader wondering what the appropriate lock level is.

Any objections to unifying this lock management in libsas to relieve
drivers of implementing this differently/incorrectly?  I'll send a
patch.

Also, we have seen the dev->sata_dev.ap == NULL problem as well.  Have
not fully tracked it down (are we issuing i/o before
sas_ata_init_host_and_port?).  But again, shouldn't libsas be making
guarantees about the state of the sata device?  At least pm8001 and
current isci still get this wrong.  And it would be better to fix it
once for all drivers.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/