* Re: [PATCH] sd: add support for TCG OPAL self encrypting disks
From: Christoph Hellwig @ 2017-06-26 11:10 UTC (permalink / raw)
To: Tejun Heo, Martin K . Petersen
Cc: Scott Bauer, Jonathan Derrick, Rafael Antognolli, Robert Elliott,
linux-ide, linux-block, linux-scsi
In-Reply-To: <20170619122646.32666-2-hch@lst.de>
ping?
On Mon, Jun 19, 2017 at 02:26:46PM +0200, Christoph Hellwig wrote:
> Just wire up the generic TCG OPAL infrastructure to the SCSI disk driver
> and the Security In/Out commands.
>
> Note that I don't know of any actual SCSI disks that do support TCG OPAL,
> but this is required to support ATA disks through libata.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/ata/libata-scsi.c | 3 +++
> drivers/scsi/sd.c | 53 +++++++++++++++++++++++++++++++++++++++++++++-
> drivers/scsi/sd.h | 2 ++
> include/scsi/scsi_device.h | 1 +
> 4 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 0f788ad6f2f6..3e5ca2e894a4 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1321,6 +1321,9 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
>
> blk_queue_flush_queueable(q, false);
>
> + if (dev->flags & ATA_DFLAG_TRUSTED)
> + sdev->security_supported = 1;
> +
> dev->sdev = sdev;
> return 0;
> }
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index f9d1432d7cc5..5d32fd7d3a3e 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -50,6 +50,7 @@
> #include <linux/string_helpers.h>
> #include <linux/async.h>
> #include <linux/slab.h>
> +#include <linux/sed-opal.h>
> #include <linux/pm_runtime.h>
> #include <linux/pr.h>
> #include <linux/t10-pi.h>
> @@ -643,6 +644,26 @@ static void scsi_disk_put(struct scsi_disk *sdkp)
> mutex_unlock(&sd_ref_mutex);
> }
>
> +#ifdef CONFIG_BLK_SED_OPAL
> +static int sd_sec_submit(void *data, u16 spsp, u8 secp, void *buffer,
> + size_t len, bool send)
> +{
> + struct scsi_device *sdev = data;
> + u8 cdb[12] = { 0, };
> + int ret;
> +
> + cdb[0] = send ? SECURITY_PROTOCOL_OUT : SECURITY_PROTOCOL_IN;
> + cdb[1] = secp;
> + put_unaligned_be16(spsp, &cdb[2]);
> + put_unaligned_be32(len, &cdb[6]);
> +
> + ret = scsi_execute_req(sdev, cdb,
> + send ? DMA_TO_DEVICE : DMA_FROM_DEVICE,
> + buffer, len, NULL, SD_TIMEOUT, SD_MAX_RETRIES, NULL);
> + return ret <= 0 ? ret : -EIO;
> +}
> +#endif /* CONFIG_BLK_SED_OPAL */
> +
> static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd,
> unsigned int dix, unsigned int dif)
> {
> @@ -1439,6 +1460,9 @@ static int sd_ioctl(struct block_device *bdev, fmode_t mode,
> if (error)
> goto out;
>
> + if (is_sed_ioctl(cmd))
> + return sed_ioctl(sdkp->opal_dev, cmd, p);
> +
> /*
> * Send SCSI addressing ioctls directly to mid level, send other
> * ioctls to block level and then onto mid level if they can't be
> @@ -2994,6 +3018,20 @@ static void sd_read_write_same(struct scsi_disk *sdkp, unsigned char *buffer)
> sdkp->ws10 = 1;
> }
>
> +static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer)
> +{
> + struct scsi_device *sdev = sdkp->device;
> +
> + if (!sdev->security_supported)
> + return;
> +
> + if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> + SECURITY_PROTOCOL_IN) == 1 &&
> + scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> + SECURITY_PROTOCOL_OUT) == 1)
> + sdkp->security = 1;
> +}
> +
> /**
> * sd_revalidate_disk - called the first time a new disk is seen,
> * performs disk spin up, read_capacity, etc.
> @@ -3047,6 +3085,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> sd_read_cache_type(sdkp, buffer);
> sd_read_app_tag_own(sdkp, buffer);
> sd_read_write_same(sdkp, buffer);
> + sd_read_security(sdkp, buffer);
> }
>
> sdkp->first_scan = 0;
> @@ -3207,6 +3246,12 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>
> sd_revalidate_disk(gd);
>
> + if (sdkp->security) {
> + sdkp->opal_dev = init_opal_dev(sdp, &sd_sec_submit);
> + if (sdkp->opal_dev)
> + sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
> + }
> +
> sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
> sdp->removable ? "removable " : "");
> scsi_autopm_put_device(sdp);
> @@ -3356,6 +3401,8 @@ static int sd_remove(struct device *dev)
>
> sd_zbc_remove(sdkp);
>
> + free_opal_dev(sdkp->opal_dev);
> +
> blk_register_region(devt, SD_MINORS, NULL,
> sd_default_probe, NULL, NULL);
>
> @@ -3497,6 +3544,7 @@ static int sd_suspend_runtime(struct device *dev)
> static int sd_resume(struct device *dev)
> {
> struct scsi_disk *sdkp = dev_get_drvdata(dev);
> + int ret;
>
> if (!sdkp) /* E.g.: runtime resume at the start of sd_probe() */
> return 0;
> @@ -3505,7 +3553,10 @@ static int sd_resume(struct device *dev)
> return 0;
>
> sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> - return sd_start_stop_device(sdkp, 1);
> + ret = sd_start_stop_device(sdkp, 1);
> + if (!ret)
> + opal_unlock_from_suspend(sdkp->opal_dev);
> + return ret;
> }
>
> /**
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 61d02efd366c..99c4dde9b6bf 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -71,6 +71,7 @@ struct scsi_disk {
> struct scsi_device *device;
> struct device dev;
> struct gendisk *disk;
> + struct opal_dev *opal_dev;
> #ifdef CONFIG_BLK_DEV_ZONED
> unsigned int nr_zones;
> unsigned int zone_blocks;
> @@ -114,6 +115,7 @@ struct scsi_disk {
> unsigned rc_basis: 2;
> unsigned zoned: 2;
> unsigned urswrz : 1;
> + unsigned security : 1;
> unsigned ignore_medium_access_errors : 1;
> };
> #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 05641aebd181..310c86a892e9 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -176,6 +176,7 @@ struct scsi_device {
> unsigned no_read_disc_info:1; /* Avoid READ_DISC_INFO cmds */
> unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
> unsigned try_rc_10_first:1; /* Try READ_CAPACACITY_10 first */
> + unsigned security_supported:1; /* Supports Security Protocols */
> unsigned is_visible:1; /* is the device visible in sysfs */
> unsigned wce_default_on:1; /* Cache is ON by default */
> unsigned no_dif:1; /* T10 PI (DIF) should be disabled */
> --
> 2.11.0
---end quoted text---
^ permalink raw reply
* Re: [PATCH] sd: add support for TCG OPAL self encrypting disks
From: Martin K. Petersen @ 2017-06-26 16:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Tejun Heo, Martin K . Petersen, Scott Bauer, Jonathan Derrick,
Rafael Antognolli, Robert Elliott, linux-ide, linux-block,
linux-scsi
In-Reply-To: <20170626111047.GA16273@lst.de>
Christoph,
> ping?
Looks good to me. I'll queue it up for 4.13 as soon as Linus has pulled
in the ata bits.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH] sd: add support for TCG OPAL self encrypting disks
From: Tejun Heo @ 2017-06-26 20:52 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Christoph Hellwig, Scott Bauer, Jonathan Derrick,
Rafael Antognolli, Robert Elliott, linux-ide, linux-block,
linux-scsi
In-Reply-To: <yq1tw327l8w.fsf@oracle.com>
On Mon, Jun 26, 2017 at 12:43:27PM -0400, Martin K. Petersen wrote:
>
> Christoph,
>
> > ping?
>
> Looks good to me. I'll queue it up for 4.13 as soon as Linus has pulled
> in the ata bits.
I can route it through libata tree w/ your ack if that's more convenient.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] sata_via: Enable optional hotplug on VT6420
From: Tejun Heo @ 2017-06-26 20:56 UTC (permalink / raw)
To: Ondrej Zary; +Cc: linux-ide, Kernel development list
In-Reply-To: <20170625202536.14925-1-linux@rainbow-software.org>
On Sun, Jun 25, 2017 at 10:25:36PM +0200, Ondrej Zary wrote:
> VT6420 seems to have the same hotplug capability as VT6421.
>
> However, enabling hotplug needs to expose SCR registers which can cause
> problems. It works for me but might break elsewhere. So add a module
> parameter vt6420_hotplug to enable this feature.
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
Applied to libata/for-4.13.
Thanks.
--
tejun
^ permalink raw reply
* Product Inquiry
From: Julian Smith @ 2017-06-26 21:51 UTC (permalink / raw)
To: linux-ide
Hello,
My name is Ms Julian Smith and i am from Sinara Group Co.
We are glad to know about your company from the web and we are interested in
your products.Please send us your Latest catalog and price list for our
trial order.
Julian Smith,
Purchasing Manager
Sinara Group Co.
7 Krasnogo Znameni Ave.
Vladivostok,690106,Ukraine
^ permalink raw reply
* [PATCH] ahci: Add Device ID for ASMedia 1061R and 1062R
From: Shawn Lin @ 2017-06-27 3:53 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, Shawn Lin
Adding ASMedia 1061R and 1062R platform device IDs for SATA.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/ata/ahci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1e1c355..5a5fd0b 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -548,6 +548,8 @@ static int ahci_p5wdh_hardreset(struct ata_link *link, unsigned int *class,
{ PCI_VDEVICE(ASMEDIA, 0x0602), board_ahci }, /* ASM1060 */
{ PCI_VDEVICE(ASMEDIA, 0x0611), board_ahci }, /* ASM1061 */
{ PCI_VDEVICE(ASMEDIA, 0x0612), board_ahci }, /* ASM1062 */
+ { PCI_VDEVICE(ASMEDIA, 0x0621), board_ahci }, /* ASM1061R */
+ { PCI_VDEVICE(ASMEDIA, 0x0622), board_ahci }, /* ASM1062R */
/*
* Samsung SSDs found on some macbooks. NCQ times out if MSI is
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] ahci: Add Device ID for ASMedia 1061R and 1062R
From: Tejun Heo @ 2017-06-27 15:23 UTC (permalink / raw)
To: Shawn Lin; +Cc: linux-ide
In-Reply-To: <1498535594-138680-1-git-send-email-shawn.lin@rock-chips.com>
On Tue, Jun 27, 2017 at 11:53:14AM +0800, Shawn Lin wrote:
> Adding ASMedia 1061R and 1062R platform device IDs for SATA.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Applied to libata/for-4.13.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] libata: Support for an ATA PASS-THROUGH(32) command.
From: Tejun Heo @ 2017-06-27 15:27 UTC (permalink / raw)
To: Minwoo Im; +Cc: James E.J. Bottomley, Martin K. Petersen, linux-ide, linux-scsi
In-Reply-To: <1498243270-21730-1-git-send-email-dn3108@gmail.com>
On Sat, Jun 24, 2017 at 03:41:10AM +0900, Minwoo Im wrote:
> SAT-4(SCSI/ATA Translation) supports for an ata pass-thru(32).
> This patch will allow to translate an ata pass-thru(32) SCSI cmd
> to an ATA cmd.
>
> Signed-off-by: Minwoo Im <dn3108@gmail.com>
> Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
Applied to libata/for-4.13.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] sd: add support for TCG OPAL self encrypting disks
From: Martin K. Petersen @ 2017-06-28 1:02 UTC (permalink / raw)
To: Tejun Heo
Cc: Martin K. Petersen, Christoph Hellwig, Scott Bauer,
Jonathan Derrick, Rafael Antognolli, Robert Elliott, linux-ide,
linux-block, linux-scsi
In-Reply-To: <20170626205220.GA27428@htj.duckdns.org>
Tejun,
>> Looks good to me. I'll queue it up for 4.13 as soon as Linus has pulled
>> in the ata bits.
>
> I can route it through libata tree w/ your ack if that's more convenient.
I don't think it will cause any headaches. I'm just a bit cautious since
I already have a ton of conflicts in linux-next for this merge window.
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH] sd: add support for TCG OPAL self encrypting disks
From: Tejun Heo @ 2017-06-28 18:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Martin K . Petersen, Scott Bauer, Jonathan Derrick,
Rafael Antognolli, Robert Elliott, linux-ide, linux-block,
linux-scsi
In-Reply-To: <20170619122646.32666-2-hch@lst.de>
On Mon, Jun 19, 2017 at 02:26:46PM +0200, Christoph Hellwig wrote:
> Just wire up the generic TCG OPAL infrastructure to the SCSI disk driver
> and the Security In/Out commands.
>
> Note that I don't know of any actual SCSI disks that do support TCG OPAL,
> but this is required to support ATA disks through libata.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Applied to libata/for-4.13 w/ Martin's ack added.
Thanks.
--
tejun
^ permalink raw reply
* [PATCH] libata: fix build warning in ata_scsi_var_len_cdb_xlat
From: Arnd Bergmann @ 2017-06-28 20:04 UTC (permalink / raw)
To: Tejun Heo
Cc: Bart Van Assche, Arnd Bergmann, Tom Yan, Hannes Reinecke,
Shaun Tancheff, Adam Manzanares, Minwoo Im, linux-ide,
linux-kernel
This new harmless warning just showed up:
drivers/ata/libata-scsi.c: In function 'ata_scsi_var_len_cdb_xlat':
drivers/ata/libata-scsi.c:4194:1: error: label 'unspprt_sa' defined but not used [-Werror=unused-label]
The label is obviously unused and can be removed.
Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/ata/libata-scsi.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 815c6e240aea..9d1b66b4ce7b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4191,7 +4191,6 @@ static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc)
if (sa == ATA_32)
return ata_scsi_pass_thru(qc);
-unspprt_sa:
/* unsupported service action */
return 1;
}
--
2.9.0
^ permalink raw reply related
* Re: [PATCH] libata: fix build warning in ata_scsi_var_len_cdb_xlat
From: Tejun Heo @ 2017-06-28 21:25 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Bart Van Assche, Tom Yan, Hannes Reinecke, Shaun Tancheff,
Adam Manzanares, Minwoo Im, linux-ide, linux-kernel
In-Reply-To: <20170628200421.3047383-1-arnd@arndb.de>
Hello, Arnd.
On Wed, Jun 28, 2017 at 10:04:03PM +0200, Arnd Bergmann wrote:
> This new harmless warning just showed up:
>
> drivers/ata/libata-scsi.c: In function 'ata_scsi_var_len_cdb_xlat':
> drivers/ata/libata-scsi.c:4194:1: error: label 'unspprt_sa' defined but not used [-Werror=unused-label]
>
> The label is obviously unused and can be removed.
>
> Fixes: b1ffbf854e08 ("libata: Support for an ATA PASS-THROUGH(32) command.")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Already fixed a couple hours ago. Sorry about the noise.
Thanks.
--
tejun
^ permalink raw reply
* spin_unlock_wait() in ata_scsi_cmd_error_handler()?
From: Paul E. McKenney @ 2017-06-29 18:10 UTC (permalink / raw)
To: tj; +Cc: linux-ide, linux-kernel
Hello, Tejun!
We are having some discussion about the semantics of spin_unlock_wait(),
and your code has one of them.
(https://marc.info/?l=linux-kernel&m=149730349001044)
We seem to agree that spin_unlock_wait() should provide acquire semantics.
Consider the following admittedly bizarre code fragment:
CPU 0 CPU 1
----- -----
spin_unlock_wait(&ml); /* Lock held initially. */
WRITE_ONCE(x, 1); r2 = READ_ONCE(x);
r1 = READ_ONCE(y); WRITE_ONCE(y, 1);
spin_unlock(&ml);
r1 == 0 || r2 == 1 /* again, evaluated "at the end of time" */
CPU 0's spin_unlock_wait() must wait for CPU 1 to release the lock,
which means that CPU 0's memory references must see the result of
CPU 1's memory references and not vice versa. In other words, the
expression beneath the code fragment cannot hold.
The current sense is that spin_unlock_wait() will -not- provide
release semantics. This calls for an even more bizarre code fragment:
CPU 0 CPU 1
----- -----
WRITE_ONCE(x, 1); spin_lock(&ml);
r1 = READ_ONCE(y); r2 = READ_ONCE(x);
spin_unlock_wait(&ml); WRITE_ONCE(y, 1);
WRITE_ONCE(z, 1); /* Intentionally not releasing lock! */
z == 1 && (r1 == 1 || r2 == 0) /* evaluated "at the end of time" */
If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait()
must have executed before CPU 1's spin_lock(). However, even on x86,
CPU 0's prior writes can be reordered with its subsequent reads, which
means that r1 == 0 is possible, which means that the above condition
could hold, even on x86.
One of the uses of spin_unlock_wait() is in ata_scsi_cmd_error_handler()
in the file drivers/ata/libata-eh.c. Your commit ad9e27624479b
("libata-eh-fw: update ata_scsi_error() for new EH") last touched it,
though it predates that commit.
My question to you is whether the code in ata_scsi_cmd_error_handler()
needs release semantics. If it does, my recommendation is to replace
the spin_unlock_wait(ap->lock) with this (adding the needed curly braces,
of course):
spin_lock(ap->lock);
spin_unlock(ap->lock);
If the code only needs acquire semantics, no change required.
If your code requires release semantics, and there is some reason why
my suggested replacement above is a bad idea, please let me know!
Thanx, Paul
^ permalink raw reply
* Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?
From: Tejun Heo @ 2017-06-29 19:53 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: linux-ide, linux-kernel
In-Reply-To: <20170629181057.GA5228@linux.vnet.ibm.com>
Hello, Paul.
On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote:
> If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait()
> must have executed before CPU 1's spin_lock(). However, even on x86,
> CPU 0's prior writes can be reordered with its subsequent reads, which
> means that r1 == 0 is possible, which means that the above condition
> could hold, even on x86.
I see. Ah, that's a mind bender.
> One of the uses of spin_unlock_wait() is in ata_scsi_cmd_error_handler()
> in the file drivers/ata/libata-eh.c. Your commit ad9e27624479b
> ("libata-eh-fw: update ata_scsi_error() for new EH") last touched it,
> though it predates that commit.
>
> My question to you is whether the code in ata_scsi_cmd_error_handler()
> needs release semantics. If it does, my recommendation is to replace
> the spin_unlock_wait(ap->lock) with this (adding the needed curly braces,
> of course):
>
> spin_lock(ap->lock);
> spin_unlock(ap->lock);
>
> If the code only needs acquire semantics, no change required.
>
> If your code requires release semantics, and there is some reason why
> my suggested replacement above is a bad idea, please let me know!
That part of the code should be dead now. I don't think we no longer
have any driver which doesn't have error handler set. I should rip
out that if/else. Also, ACQUIRE semantics should be enough there.
Nothing changes from the EH side there.
Thanks.
--
tejun
^ permalink raw reply
* Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?
From: Paul E. McKenney @ 2017-06-29 20:14 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, linux-kernel
In-Reply-To: <20170629195322.GB9745@htj.duckdns.org>
On Thu, Jun 29, 2017 at 03:53:22PM -0400, Tejun Heo wrote:
> Hello, Paul.
>
> On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote:
> > If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait()
> > must have executed before CPU 1's spin_lock(). However, even on x86,
> > CPU 0's prior writes can be reordered with its subsequent reads, which
> > means that r1 == 0 is possible, which means that the above condition
> > could hold, even on x86.
>
> I see. Ah, that's a mind bender.
It has indeed been providing at least its share of entertainment over
the past little while. ;-)
> > One of the uses of spin_unlock_wait() is in ata_scsi_cmd_error_handler()
> > in the file drivers/ata/libata-eh.c. Your commit ad9e27624479b
> > ("libata-eh-fw: update ata_scsi_error() for new EH") last touched it,
> > though it predates that commit.
> >
> > My question to you is whether the code in ata_scsi_cmd_error_handler()
> > needs release semantics. If it does, my recommendation is to replace
> > the spin_unlock_wait(ap->lock) with this (adding the needed curly braces,
> > of course):
> >
> > spin_lock(ap->lock);
> > spin_unlock(ap->lock);
> >
> > If the code only needs acquire semantics, no change required.
> >
> > If your code requires release semantics, and there is some reason why
> > my suggested replacement above is a bad idea, please let me know!
>
> That part of the code should be dead now. I don't think we no longer
> have any driver which doesn't have error handler set. I should rip
> out that if/else. Also, ACQUIRE semantics should be enough there.
> Nothing changes from the EH side there.
It looks like we actually might get rid of spin_unlock_wait entirely.
But how about if I just pull the spin_lock_irqsave() before the "if"
and the spin_lock_irqrestore() after the "if"? Same effect, only
difference is that the "if" and the "ap->eh_tries = ATA_EH_MAX_TRIES"
end up under the lock, and I bet that you won't be able to measure
the difference. (Please see below.)
I will do this because I just now happened to be editing that file on
my "eradicate spin_unlock_wait()" quest, but can easily rework the
patch as desired. If you want something different, just let me know!
Thanx, Paul
------------------------------------------------------------------------
commit 39a15ef3b324b08606953d519e9bc538318f3c15
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date: Thu Jun 29 13:10:47 2017 -0700
drivers/ata: Replace spin_unlock_wait() with lock/unlock pair
There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair. This commit therefore eliminates the spin_unlock_wait() call and
associated else-clause and hoists the then-clause's lock and unlock out of
the "if" statement. This should be safe from a performance perspective
because according to Tejun there should be few if any drivers that don't
set their own error handler.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: <linux-ide@vger.kernel.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrea Parri <parri.andrea@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index ef68232b5222..779f6f18c1f4 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -645,12 +645,11 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
* completions are honored. A scmd is determined to have
* timed out iff its associated qc is active and not failed.
*/
+ spin_lock_irqsave(ap->lock, flags);
if (ap->ops->error_handler) {
struct scsi_cmnd *scmd, *tmp;
int nr_timedout = 0;
- spin_lock_irqsave(ap->lock, flags);
-
/* This must occur under the ap->lock as we don't want
a polled recovery to race the real interrupt handler
@@ -700,12 +699,11 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
if (nr_timedout)
__ata_port_freeze(ap);
- spin_unlock_irqrestore(ap->lock, flags);
/* initialize eh_tries */
ap->eh_tries = ATA_EH_MAX_TRIES;
- } else
- spin_unlock_wait(ap->lock);
+ }
+ spin_unlock_irqrestore(ap->lock, flags);
}
EXPORT_SYMBOL(ata_scsi_cmd_error_handler);
^ permalink raw reply related
* Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?
From: Tejun Heo @ 2017-06-29 20:17 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: linux-ide, linux-kernel
In-Reply-To: <20170629201443.GD2393@linux.vnet.ibm.com>
Hello,
On Thu, Jun 29, 2017 at 01:14:43PM -0700, Paul E. McKenney wrote:
> On Thu, Jun 29, 2017 at 03:53:22PM -0400, Tejun Heo wrote:
> > Hello, Paul.
> >
> > On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote:
> > > If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait()
> > > must have executed before CPU 1's spin_lock(). However, even on x86,
> > > CPU 0's prior writes can be reordered with its subsequent reads, which
> > > means that r1 == 0 is possible, which means that the above condition
> > > could hold, even on x86.
> >
> > I see. Ah, that's a mind bender.
>
> It has indeed been providing at least its share of entertainment over
> the past little while. ;-)
lol :)
> > That part of the code should be dead now. I don't think we no longer
> > have any driver which doesn't have error handler set. I should rip
> > out that if/else. Also, ACQUIRE semantics should be enough there.
> > Nothing changes from the EH side there.
>
> It looks like we actually might get rid of spin_unlock_wait entirely.
> But how about if I just pull the spin_lock_irqsave() before the "if"
> and the spin_lock_irqrestore() after the "if"? Same effect, only
> difference is that the "if" and the "ap->eh_tries = ATA_EH_MAX_TRIES"
> end up under the lock, and I bet that you won't be able to measure
> the difference. (Please see below.)
>
> I will do this because I just now happened to be editing that file on
> my "eradicate spin_unlock_wait()" quest, but can easily rework the
> patch as desired. If you want something different, just let me know!
Sounds good to me. That path isn't hot at all. No change made at
this level is gonna have any actual impact. Please go for whatever is
the simplest. For moving out the lock/unlock outside if/else,
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: spin_unlock_wait() in ata_scsi_cmd_error_handler()?
From: Paul E. McKenney @ 2017-06-29 20:48 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, linux-kernel
In-Reply-To: <20170629201754.GC9745@htj.duckdns.org>
On Thu, Jun 29, 2017 at 04:17:54PM -0400, Tejun Heo wrote:
> Hello,
>
> On Thu, Jun 29, 2017 at 01:14:43PM -0700, Paul E. McKenney wrote:
> > On Thu, Jun 29, 2017 at 03:53:22PM -0400, Tejun Heo wrote:
> > > Hello, Paul.
> > >
> > > On Thu, Jun 29, 2017 at 11:10:57AM -0700, Paul E. McKenney wrote:
> > > > If this code fragment doesn't deadlock, then CPU 0's spin_unlock_wait()
> > > > must have executed before CPU 1's spin_lock(). However, even on x86,
> > > > CPU 0's prior writes can be reordered with its subsequent reads, which
> > > > means that r1 == 0 is possible, which means that the above condition
> > > > could hold, even on x86.
> > >
> > > I see. Ah, that's a mind bender.
> >
> > It has indeed been providing at least its share of entertainment over
> > the past little while. ;-)
>
> lol :)
>
> > > That part of the code should be dead now. I don't think we no longer
> > > have any driver which doesn't have error handler set. I should rip
> > > out that if/else. Also, ACQUIRE semantics should be enough there.
> > > Nothing changes from the EH side there.
> >
> > It looks like we actually might get rid of spin_unlock_wait entirely.
> > But how about if I just pull the spin_lock_irqsave() before the "if"
> > and the spin_lock_irqrestore() after the "if"? Same effect, only
> > difference is that the "if" and the "ap->eh_tries = ATA_EH_MAX_TRIES"
> > end up under the lock, and I bet that you won't be able to measure
> > the difference. (Please see below.)
> >
> > I will do this because I just now happened to be editing that file on
> > my "eradicate spin_unlock_wait()" quest, but can easily rework the
> > patch as desired. If you want something different, just let me know!
>
> Sounds good to me. That path isn't hot at all. No change made at
> this level is gonna have any actual impact. Please go for whatever is
> the simplest. For moving out the lock/unlock outside if/else,
>
> Acked-by: Tejun Heo <tj@kernel.org>
Applied, and thank you!
Thanx, Paul
^ permalink raw reply
* [PATCH RFC 07/26] drivers/ata: Replace spin_unlock_wait() with lock/unlock pair
From: Paul E. McKenney @ 2017-06-30 0:01 UTC (permalink / raw)
To: linux-kernel
Cc: netfilter-devel, netdev, oleg, akpm, mingo, dave, manfred, tj,
arnd, linux-arch, will.deacon, peterz, stern, parri.andrea,
torvalds, Paul E. McKenney, linux-ide
In-Reply-To: <20170629235918.GA6445@linux.vnet.ibm.com>
There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair. This commit therefore eliminates the spin_unlock_wait() call and
associated else-clause and hoists the then-clause's lock and unlock out of
the "if" statement. This should be safe from a performance perspective
because according to Tejun there should be few if any drivers that don't
set their own error handler.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Acked-by: Tejun Heo <tj@kernel.org>
Cc: <linux-ide@vger.kernel.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrea Parri <parri.andrea@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
drivers/ata/libata-eh.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index ef68232b5222..779f6f18c1f4 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -645,12 +645,11 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
* completions are honored. A scmd is determined to have
* timed out iff its associated qc is active and not failed.
*/
+ spin_lock_irqsave(ap->lock, flags);
if (ap->ops->error_handler) {
struct scsi_cmnd *scmd, *tmp;
int nr_timedout = 0;
- spin_lock_irqsave(ap->lock, flags);
-
/* This must occur under the ap->lock as we don't want
a polled recovery to race the real interrupt handler
@@ -700,12 +699,11 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
if (nr_timedout)
__ata_port_freeze(ap);
- spin_unlock_irqrestore(ap->lock, flags);
/* initialize eh_tries */
ap->eh_tries = ATA_EH_MAX_TRIES;
- } else
- spin_unlock_wait(ap->lock);
+ }
+ spin_unlock_irqrestore(ap->lock, flags);
}
EXPORT_SYMBOL(ata_scsi_cmd_error_handler);
--
2.5.2
^ permalink raw reply related
* [PATCH] sata_highbank: fix error return code in ahci_highbank_probe()
From: Gustavo A. R. Silva @ 2017-06-30 5:03 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, linux-kernel, Gustavo A. R. Silva
Propagate the return value of platform_get_irq on failure.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/ata/sata_highbank.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index aafb8cc..2fc451c 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -483,9 +483,9 @@ static int ahci_highbank_probe(struct platform_device *pdev)
}
irq = platform_get_irq(pdev, 0);
- if (irq <= 0) {
+ if (irq < 0) {
dev_err(dev, "no irq\n");
- return -EINVAL;
+ return irq;
}
hpriv = devm_kzalloc(dev, sizeof(*hpriv), GFP_KERNEL);
--
2.5.0
^ permalink raw reply related
* [PATCH] pata_imx: print error message on platform_get_irq failure
From: Gustavo A. R. Silva @ 2017-06-30 5:29 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, linux-kernel, Gustavo A. R. Silva
Print error message on platform_get_irq failure before return.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/ata/pata_imx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/pata_imx.c b/drivers/ata/pata_imx.c
index d4caa23..65bbf36 100644
--- a/drivers/ata/pata_imx.c
+++ b/drivers/ata/pata_imx.c
@@ -132,8 +132,10 @@ static int pata_imx_probe(struct platform_device *pdev)
int ret;
irq = platform_get_irq(pdev, 0);
- if (irq < 0)
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ\n");
return irq;
+ }
priv = devm_kzalloc(&pdev->dev,
sizeof(struct pata_imx_priv), GFP_KERNEL);
--
2.5.0
^ permalink raw reply related
* [PATCH] sata_rcar: fix error return code in sata_rcar_probe()
From: Gustavo A. R. Silva @ 2017-06-30 5:22 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, linux-kernel, Gustavo A. R. Silva
Print error message and propagate the return value of
platform_get_irq on failure.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
drivers/ata/sata_rcar.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
index ee98447..c936b2a 100644
--- a/drivers/ata/sata_rcar.c
+++ b/drivers/ata/sata_rcar.c
@@ -872,8 +872,10 @@ static int sata_rcar_probe(struct platform_device *pdev)
int ret = 0;
irq = platform_get_irq(pdev, 0);
- if (irq <= 0)
- return -EINVAL;
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ\n");
+ return irq;
+ }
priv = devm_kzalloc(&pdev->dev, sizeof(struct sata_rcar_priv),
GFP_KERNEL);
--
2.5.0
^ permalink raw reply related
* Re: [PATCH] sata_rcar: fix error return code in sata_rcar_probe()
From: Sergei Shtylyov @ 2017-06-30 9:42 UTC (permalink / raw)
To: Gustavo A. R. Silva, Tejun Heo; +Cc: linux-ide, linux-kernel
In-Reply-To: <20170630052210.GA19186@embeddedgus>
Hello!
On 6/30/2017 8:22 AM, Gustavo A. R. Silva wrote:
> Print error message and propagate the return value of
> platform_get_irq on failure.
You should have probably mentioned that this function no longer returns 0
on error.
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] sata_rcar: fix error return code in sata_rcar_probe()
From: Tejun Heo @ 2017-06-30 12:01 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: Gustavo A. R. Silva, linux-ide, linux-kernel
In-Reply-To: <e71b065e-554e-4dee-06cc-b1cea6b3de53@cogentembedded.com>
On Fri, Jun 30, 2017 at 12:42:38PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 6/30/2017 8:22 AM, Gustavo A. R. Silva wrote:
>
> > Print error message and propagate the return value of
> > platform_get_irq on failure.
>
> You should have probably mentioned that this function no longer returns 0
> on error.
Yeah, the patches looks good to me but I'd really appreciate more
context in the changelogs. Gustavo, can you please respin the
patches?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] sata_rcar: fix error return code in sata_rcar_probe()
From: Sergei Shtylyov @ 2017-06-30 12:35 UTC (permalink / raw)
To: Gustavo A. R. Silva, Tejun Heo; +Cc: linux-ide, linux-kernel
In-Reply-To: <e71b065e-554e-4dee-06cc-b1cea6b3de53@cogentembedded.com>
On 06/30/2017 12:42 PM, Sergei Shtylyov wrote:
>> Print error message and propagate the return value of
>> platform_get_irq on failure.
>
> You should have probably mentioned that this function no longer returns 0
> on error.
It's prolly also worth mentioning that enforcing the error # on return
from probe defeats the deferred probing.
MBR, Sergei
^ permalink raw reply
* [PATCH V5 0/3] ADD AHCI support for tegra210
From: Preetham Chandru Ramchandra @ 2017-06-30 13:56 UTC (permalink / raw)
To: thierry.reding-Re5JQEeQqe8AvxtiuMwx3w, tj-DgEjT+Ai2ygdnm+yROfE0A,
cyndis-/1wQRMveznE
Cc: preetham260-Re5JQEeQqe8AvxtiuMwx3w,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-ide-u79uwXL29TY76Z2rM5mHXA,
vbyravarasu-DDmLM1+adcrQT0dZR+AlfA,
pkunapuli-DDmLM1+adcrQT0dZR+AlfA, Preetham Chandru R
From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
1. ADD AHCI support for tegra210
2. Extend the tegra AHCI controller device tree binding with tegra210
---
Preetham Chandru R (3):
ata: ahci_tegra: Add AHCI support for tegra210
arm64: tegra: Enable AHCI on Tegra210
dt-bindings: tegra: add binding documentation
.../bindings/ata/nvidia,tegra124-ahci.txt | 38 ++-
arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 6 +
arch/arm64/boot/dts/nvidia/tegra210.dtsi | 16 +
drivers/ata/ahci_tegra.c | 379 ++++++++++++++++-----
4 files changed, 343 insertions(+), 96 deletions(-)
--
2.1.4
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox