* Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings
From: Tejun Heo @ 2017-01-09 17:31 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Geert Uytterhoeven, linux-ide@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20170109172723.GA30423@infradead.org>
Hello,
On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
> > > ata_force_param_buf is __initdata and shouldn't really matter.
> >
> > It mainly matters because of e.g. bootloader limitations.
>
> Do we need a full 4k for the force parameters? What would a typical
> command line for it look like?
Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
this given that it is bss, not gigantic and __initdata. What kind of
bootloader limitations are we talking about?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings
From: Geert Uytterhoeven @ 2017-01-09 18:25 UTC (permalink / raw)
To: Tejun Heo
Cc: Christoph Hellwig, linux-ide@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20170109173154.GE12827@mtj.duckdns.org>
On Mon, Jan 9, 2017 at 6:31 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
>> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
>> > > ata_force_param_buf is __initdata and shouldn't really matter.
>> >
>> > It mainly matters because of e.g. bootloader limitations.
>>
>> Do we need a full 4k for the force parameters? What would a typical
>> command line for it look like?
>
> Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
> this given that it is bss, not gigantic and __initdata. What kind of
> bootloader limitations are we talking about?
Some boot loaders start overwriting themselves or the passed DTB if the
kernel becomes too big.
If I'm not mistaken, bss is still expanded early (verified, increasing bss
can trigger the above problem).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings
From: Tejun Heo @ 2017-01-09 19:41 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Christoph Hellwig, linux-ide@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdXfFAyyT2VWwmG3sHu_Ufqzoz39O95rXDUCvHaD9HQFZA@mail.gmail.com>
Hello, Geert.
On Mon, Jan 09, 2017 at 07:25:31PM +0100, Geert Uytterhoeven wrote:
> On Mon, Jan 9, 2017 at 6:31 PM, Tejun Heo <tj@kernel.org> wrote:
> > On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
> >> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
> >> > > ata_force_param_buf is __initdata and shouldn't really matter.
> >> >
> >> > It mainly matters because of e.g. bootloader limitations.
> >>
> >> Do we need a full 4k for the force parameters? What would a typical
> >> command line for it look like?
> >
> > Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
> > this given that it is bss, not gigantic and __initdata. What kind of
> > bootloader limitations are we talking about?
>
> Some boot loaders start overwriting themselves or the passed DTB if the
> kernel becomes too big.
> If I'm not mistaken, bss is still expanded early (verified, increasing bss
> can trigger the above problem).
So, to avoid that, we can just kmalloc and kfree the buffer, but it
seems like a silly complication to work around bugs in some
bootloaders. There are many places in kernel where we're liberal
about __initdata which is great. I'm not sure complicating all those
places for a broken bootloader is a good idea.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] Fix interface autodetection in legacy IDE driver (trial #2)
From: David Miller @ 2017-01-09 20:25 UTC (permalink / raw)
To: b.zolnierkie; +Cc: lramos.prof, linux-ide, petkovbb
In-Reply-To: <2007222.eymCZIUvPG@amdc3058>
From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Date: Fri, 30 Dec 2016 17:05:51 +0100
> Yes, it was a conscious decision.
I understand things now, I'll revert this change, thanks everyone.
^ permalink raw reply
* Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings
From: Geert Uytterhoeven @ 2017-01-09 20:28 UTC (permalink / raw)
To: Tejun Heo
Cc: Christoph Hellwig, linux-ide@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20170109194100.GH12827@mtj.duckdns.org>
Hi Tejun,
On Mon, Jan 9, 2017 at 8:41 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Jan 09, 2017 at 07:25:31PM +0100, Geert Uytterhoeven wrote:
>> On Mon, Jan 9, 2017 at 6:31 PM, Tejun Heo <tj@kernel.org> wrote:
>> > On Mon, Jan 09, 2017 at 09:27:23AM -0800, Christoph Hellwig wrote:
>> >> On Mon, Jan 09, 2017 at 05:30:02PM +0100, Geert Uytterhoeven wrote:
>> >> > > ata_force_param_buf is __initdata and shouldn't really matter.
>> >> >
>> >> > It mainly matters because of e.g. bootloader limitations.
>> >>
>> >> Do we need a full 4k for the force parameters? What would a typical
>> >> command line for it look like?
>> >
>> > Maybe a couple hundreds bytes at max, but it's a bit weird to restrict
>> > this given that it is bss, not gigantic and __initdata. What kind of
>> > bootloader limitations are we talking about?
>>
>> Some boot loaders start overwriting themselves or the passed DTB if the
>> kernel becomes too big.
>> If I'm not mistaken, bss is still expanded early (verified, increasing bss
>> can trigger the above problem).
>
> So, to avoid that, we can just kmalloc and kfree the buffer, but it
> seems like a silly complication to work around bugs in some
> bootloaders. There are many places in kernel where we're liberal
> about __initdata which is great. I'm not sure complicating all those
> places for a broken bootloader is a good idea.
Sure. We cannot avoid that kernels (esp. multiplatform) keep on growing.
But when I see a new 4KiB-sized buffer, i'm always suspicious...
A few years ago, I caught someone miscalculating shifts, leading
to a static buffer that was 256 times larger than intended ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] libata-eh: Use switch() instead of sparse array for protocol strings
From: Tejun Heo @ 2017-01-09 20:31 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Christoph Hellwig, linux-ide@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdUVtQOJYfQsspSk9+v-4abGf+OTSMWEfzXvGLu9namqZg@mail.gmail.com>
Hello,
On Mon, Jan 09, 2017 at 09:28:12PM +0100, Geert Uytterhoeven wrote:
> > So, to avoid that, we can just kmalloc and kfree the buffer, but it
> > seems like a silly complication to work around bugs in some
> > bootloaders. There are many places in kernel where we're liberal
> > about __initdata which is great. I'm not sure complicating all those
> > places for a broken bootloader is a good idea.
>
> Sure. We cannot avoid that kernels (esp. multiplatform) keep on growing.
>
> But when I see a new 4KiB-sized buffer, i'm always suspicious...
> A few years ago, I caught someone miscalculating shifts, leading
> to a static buffer that was 256 times larger than intended ;-)
Oh, sure, things like the protocol string table are just stupid and
it's great that you caught it. I just don't think it makes sense to
scrutinize bss __initdata. It's not in kernel image and goes away
once the kernel boots. It's okay to a bit liberal with them for the
sake of simplicity.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 1/1] libata: Fix ATA request sense
From: Damien Le Moal @ 2017-01-10 0:54 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide, Hannes Reinecke
In-Reply-To: <20170106204416.GC29909@mtj.duckdns.org>
Tejun,
On 1/7/17 05:44, Tejun Heo wrote:
> On Mon, Dec 19, 2016 at 10:17:40AM +0900, Damien Le Moal wrote:
>> For an ATA device supporting the sense data reporting feature set,
>> a failed command will trigger the execution of ata_eh_request_sense if
>> the result task file of the failed command has the ATA_SENSE bit set
>> (sense data available bit). ata_eh_request_sense executes the
>> REQUEST SENSE DATA EXT command to retrieve the sense data of the failed
>> command. On success of REQUEST SENSE DATA EXT, the ATA_SENSE bit will
>> NOT be set (the command succeeded) but ata_eh_request_sense
>> nevertheless tests the availability of sense data by testing that bit
>> presence in the result tf of the REQUEST SENSE DATA EXT command.
>> This leads to a falsy assume that request sense data failed and to the
>> warning message:
>>
>> atax.xx: request sense failed stat 50 emask 0
>>
>> Upon success of REQUEST SENSE DATA EXT, set the ATA_SENSE bit in the
>> result task file command so that sense data can be returned by
>> ata_eh_request_sense.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>
> Applied to libata/for-4.10-fixes with patch description updated as
> suggested by Sergei.
Thank you.
Should we send this patch to stable too ?
This bug does break applications on kernels 4.7 to 4.9 (e.g. libzbc test
suite and many tools we have using SGIO to directly send ATA commands to
SATA disks). I suspect sg-utils and hdparm may be affected in some way too.
Best regards.
--
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com
^ permalink raw reply
* libata: remove the global response buffer
From: Christoph Hellwig @ 2017-01-10 8:41 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
Avoid the need for a global 4k buffer by allocating no response buffer
for commands that don't return data, a small on-stack one for the ATAPI
fixup, or a kmalloced one for all other emulated commands.
^ permalink raw reply
* [PATCH 1/6] libata: avoid global response buffer in atapi_qc_complete
From: Christoph Hellwig @ 2017-01-10 8:41 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-1-git-send-email-hch@lst.de>
We only need to look at 4 bytes of the inquiry response for ATAPI
devices. Instead of using the global ata_scsi_rbuf just use a
a stack buffer. Also factor the fixup into it's own little helper
function to make it more readable.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 46 ++++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1f863e7..031a77a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2873,6 +2873,26 @@ static void atapi_request_sense(struct ata_queued_cmd *qc)
DPRINTK("EXIT\n");
}
+/*
+ * ATAPI devices typically report zero for their SCSI version, and sometimes
+ * deviate from the spec WRT response data format. If SCSI version is
+ * reported as zero like normal, then we make the following fixups:
+ * 1) Fake MMC-5 version, to indicate to the Linux scsi midlayer this is a
+ * modern device.
+ * 2) Ensure response data format / ATAPI information are always correct.
+ */
+static void atapi_fixup_inquiry(struct scsi_cmnd *cmd)
+{
+ u8 buf[4];
+
+ sg_copy_to_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, 4);
+ if (buf[2] == 0) {
+ buf[2] = 0x5;
+ buf[3] = 0x32;
+ }
+ sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, 4);
+}
+
static void atapi_qc_complete(struct ata_queued_cmd *qc)
{
struct scsi_cmnd *cmd = qc->scsicmd;
@@ -2927,30 +2947,8 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
*/
ata_gen_passthru_sense(qc);
} else {
- u8 *scsicmd = cmd->cmnd;
-
- if ((scsicmd[0] == INQUIRY) && ((scsicmd[1] & 0x03) == 0)) {
- unsigned long flags;
- u8 *buf;
-
- buf = ata_scsi_rbuf_get(cmd, true, &flags);
-
- /* ATAPI devices typically report zero for their SCSI version,
- * and sometimes deviate from the spec WRT response data
- * format. If SCSI version is reported as zero like normal,
- * then we make the following fixups: 1) Fake MMC-5 version,
- * to indicate to the Linux scsi midlayer this is a modern
- * device. 2) Ensure response data format / ATAPI information
- * are always correct.
- */
- if (buf[2] == 0) {
- buf[2] = 0x5;
- buf[3] = 0x32;
- }
-
- ata_scsi_rbuf_put(cmd, true, &flags);
- }
-
+ if (cmd->cmnd[0] == INQUIRY && (cmd->cmnd[1] & 0x03) == 0)
+ atapi_fixup_inquiry(cmd);
cmd->result = SAM_STAT_GOOD;
}
--
2.1.4
^ permalink raw reply related
* [PATCH 2/6] libata: move struct ata_scsi_args to libata-scsi.c
From: Christoph Hellwig @ 2017-01-10 8:41 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-1-git-send-email-hch@lst.de>
It's only used in libata-scsi.c, so move it closer to the users.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 7 +++++++
drivers/ata/libata.h | 7 -------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 031a77a..43694f5 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2057,6 +2057,13 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
return SCSI_MLQUEUE_HOST_BUSY;
}
+struct ata_scsi_args {
+ struct ata_device *dev;
+ u16 *id;
+ struct scsi_cmnd *cmd;
+ void (*done)(struct scsi_cmnd *);
+};
+
/**
* ata_scsi_rbuf_get - Map response buffer.
* @cmd: SCSI command containing buffer to be mapped.
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 8f3a559..9943772 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -31,13 +31,6 @@
#define DRV_NAME "libata"
#define DRV_VERSION "3.00" /* must be exactly four chars */
-struct ata_scsi_args {
- struct ata_device *dev;
- u16 *id;
- struct scsi_cmnd *cmd;
- void (*done)(struct scsi_cmnd *);
-};
-
/* libata-core.c */
enum {
/* flags for ata_dev_read_id() */
--
2.1.4
^ permalink raw reply related
* [PATCH 3/6] libata: remove the done allback from ata_scsi_args
From: Christoph Hellwig @ 2017-01-10 8:41 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-1-git-send-email-hch@lst.de>
It's always the scsi_done callback, and we can get at that easily
in the place where ->done is called.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 43694f5..28e2530 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2061,7 +2061,6 @@ struct ata_scsi_args {
struct ata_device *dev;
u16 *id;
struct scsi_cmnd *cmd;
- void (*done)(struct scsi_cmnd *);
};
/**
@@ -2140,7 +2139,7 @@ static void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
if (rc == 0)
cmd->result = SAM_STAT_GOOD;
- args->done(cmd);
+ cmd->scsi_done(cmd);
}
/**
@@ -4357,7 +4356,6 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
args.dev = dev;
args.id = dev->id;
args.cmd = cmd;
- args.done = cmd->scsi_done;
switch(scsicmd[0]) {
case INQUIRY:
--
2.1.4
^ permalink raw reply related
* [PATCH 4/6] libata: call ->scsi_done from ata_scsi_simulate
From: Christoph Hellwig @ 2017-01-10 8:41 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-1-git-send-email-hch@lst.de>
We always need to call ->scsi_done after we've finished emulating a
command, so do it in a single place at the end of ata_scsi_simulate.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 28e2530..6078bc2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -484,13 +484,6 @@ struct device_attribute *ata_common_sdev_attrs[] = {
};
EXPORT_SYMBOL_GPL(ata_common_sdev_attrs);
-static void ata_scsi_invalid_field(struct ata_device *dev,
- struct scsi_cmnd *cmd, u16 field)
-{
- ata_scsi_set_invalid_field(dev, cmd, field, 0xff);
- cmd->scsi_done(cmd);
-}
-
/**
* ata_std_bios_param - generic bios head/sector/cylinder calculator used by sd.
* @sdev: SCSI device for which BIOS geometry is to be determined
@@ -2139,7 +2132,6 @@ static void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
if (rc == 0)
cmd->result = SAM_STAT_GOOD;
- cmd->scsi_done(cmd);
}
/**
@@ -4360,7 +4352,7 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
switch(scsicmd[0]) {
case INQUIRY:
if (scsicmd[1] & 2) /* is CmdDt set? */
- ata_scsi_invalid_field(dev, cmd, 1);
+ ata_scsi_set_invalid_field(dev, cmd, 1, 0xff);
else if ((scsicmd[1] & 1) == 0) /* is EVPD clear? */
ata_scsi_rbuf_fill(&args, ata_scsiop_inq_std);
else switch (scsicmd[2]) {
@@ -4392,7 +4384,7 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
}
/* Fallthrough */
default:
- ata_scsi_invalid_field(dev, cmd, 2);
+ ata_scsi_set_invalid_field(dev, cmd, 2, 0xff);
break;
}
break;
@@ -4410,7 +4402,7 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
if ((scsicmd[1] & 0x1f) == SAI_READ_CAPACITY_16)
ata_scsi_rbuf_fill(&args, ata_scsiop_read_cap);
else
- ata_scsi_invalid_field(dev, cmd, 1);
+ ata_scsi_set_invalid_field(dev, cmd, 1, 0xff);
break;
case REPORT_LUNS:
@@ -4420,7 +4412,6 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
case REQUEST_SENSE:
ata_scsi_set_sense(dev, cmd, 0, 0, 0);
cmd->result = (DRIVER_SENSE << 24);
- cmd->scsi_done(cmd);
break;
/* if we reach this, then writeback caching is disabled,
@@ -4442,23 +4433,24 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
if ((tmp8 == 0x4) && (!scsicmd[3]) && (!scsicmd[4]))
ata_scsi_rbuf_fill(&args, ata_scsiop_noop);
else
- ata_scsi_invalid_field(dev, cmd, 1);
+ ata_scsi_set_invalid_field(dev, cmd, 1, 0xff);
break;
case MAINTENANCE_IN:
if (scsicmd[1] == MI_REPORT_SUPPORTED_OPERATION_CODES)
ata_scsi_rbuf_fill(&args, ata_scsiop_maint_in);
else
- ata_scsi_invalid_field(dev, cmd, 1);
+ ata_scsi_set_invalid_field(dev, cmd, 1, 0xff);
break;
/* all other commands */
default:
ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x20, 0x0);
/* "Invalid command operation code" */
- cmd->scsi_done(cmd);
break;
}
+
+ cmd->scsi_done(cmd);
}
int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht)
--
2.1.4
^ permalink raw reply related
* [PATCH 5/6] libata: don't call ata_scsi_rbuf_fill for command without a response buffer
From: Christoph Hellwig @ 2017-01-10 8:41 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-1-git-send-email-hch@lst.de>
No need to copy a zeroed buffer to the caller if the command is defined
to not have a response in the SCSI spec.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 6078bc2..395c859 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2453,23 +2453,6 @@ static unsigned int ata_scsiop_inq_b6(struct ata_scsi_args *args, u8 *rbuf)
}
/**
- * ata_scsiop_noop - Command handler that simply returns success.
- * @args: device IDENTIFY data / SCSI command of interest.
- * @rbuf: Response buffer, to which simulated SCSI cmd output is sent.
- *
- * No operation. Simply returns success to caller, to indicate
- * that the caller should successfully complete this SCSI command.
- *
- * LOCKING:
- * spin_lock_irqsave(host lock)
- */
-static unsigned int ata_scsiop_noop(struct ata_scsi_args *args, u8 *rbuf)
-{
- VPRINTK("ENTER\n");
- return 0;
-}
-
-/**
* modecpy - Prepare response for MODE SENSE
* @dest: output buffer
* @src: data being copied
@@ -4425,14 +4408,11 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
case SEEK_6:
case SEEK_10:
case TEST_UNIT_READY:
- ata_scsi_rbuf_fill(&args, ata_scsiop_noop);
break;
case SEND_DIAGNOSTIC:
tmp8 = scsicmd[1] & ~(1 << 3);
- if ((tmp8 == 0x4) && (!scsicmd[3]) && (!scsicmd[4]))
- ata_scsi_rbuf_fill(&args, ata_scsiop_noop);
- else
+ if (tmp8 != 0x4 || scsicmd[3] || scsicmd[4])
ata_scsi_set_invalid_field(dev, cmd, 1, 0xff);
break;
--
2.1.4
^ permalink raw reply related
* [PATCH 6/6] libata: switch to dynamic allocation insted of ata_scsi_rbuf
From: Christoph Hellwig @ 2017-01-10 8:41 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-1-git-send-email-hch@lst.de>
Note of the emulated commands in the pageout/pagein path, so just do
a GFP_NOIO dynamic allocation.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 122 ++++++++++++++--------------------------------
1 file changed, 36 insertions(+), 86 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 395c859..785eb382 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -57,9 +57,6 @@
#define ATA_SCSI_RBUF_SIZE 4096
-static DEFINE_SPINLOCK(ata_scsi_rbuf_lock);
-static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];
-
typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc);
static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
@@ -2057,53 +2054,6 @@ struct ata_scsi_args {
};
/**
- * ata_scsi_rbuf_get - Map response buffer.
- * @cmd: SCSI command containing buffer to be mapped.
- * @flags: unsigned long variable to store irq enable status
- * @copy_in: copy in from user buffer
- *
- * Prepare buffer for simulated SCSI commands.
- *
- * LOCKING:
- * spin_lock_irqsave(ata_scsi_rbuf_lock) on success
- *
- * RETURNS:
- * Pointer to response buffer.
- */
-static void *ata_scsi_rbuf_get(struct scsi_cmnd *cmd, bool copy_in,
- unsigned long *flags)
-{
- spin_lock_irqsave(&ata_scsi_rbuf_lock, *flags);
-
- memset(ata_scsi_rbuf, 0, ATA_SCSI_RBUF_SIZE);
- if (copy_in)
- sg_copy_to_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
- ata_scsi_rbuf, ATA_SCSI_RBUF_SIZE);
- return ata_scsi_rbuf;
-}
-
-/**
- * ata_scsi_rbuf_put - Unmap response buffer.
- * @cmd: SCSI command containing buffer to be unmapped.
- * @copy_out: copy out result
- * @flags: @flags passed to ata_scsi_rbuf_get()
- *
- * Returns rbuf buffer. The result is copied to @cmd's buffer if
- * @copy_back is true.
- *
- * LOCKING:
- * Unlocks ata_scsi_rbuf_lock.
- */
-static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd, bool copy_out,
- unsigned long *flags)
-{
- if (copy_out)
- sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
- ata_scsi_rbuf, ATA_SCSI_RBUF_SIZE);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, *flags);
-}
-
-/**
* ata_scsi_rbuf_fill - wrapper for SCSI command simulators
* @args: device IDENTIFY data / SCSI command of interest.
* @actor: Callback hook for desired SCSI command simulator
@@ -2121,17 +2071,22 @@ static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd, bool copy_out,
static void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
unsigned int (*actor)(struct ata_scsi_args *args, u8 *rbuf))
{
- u8 *rbuf;
- unsigned int rc;
struct scsi_cmnd *cmd = args->cmd;
- unsigned long flags;
+ u8 *buf;
- rbuf = ata_scsi_rbuf_get(cmd, false, &flags);
- rc = actor(args, rbuf);
- ata_scsi_rbuf_put(cmd, rc == 0, &flags);
+ buf = kzalloc(ATA_SCSI_RBUF_SIZE, GFP_NOIO);
+ if (!buf) {
+ ata_scsi_set_sense(args->dev, cmd, NOT_READY, 0x08, 0);
+ return;
+ }
- if (rc == 0)
+ if (actor(args, buf) == 0) {
+ sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
+ buf, ATA_SCSI_RBUF_SIZE);
cmd->result = SAM_STAT_GOOD;
+ }
+
+ kfree(buf);
}
/**
@@ -3363,24 +3318,17 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
*
* Return: Number of bytes copied into sglist.
*/
-static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
+static ssize_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
u64 sector, u32 count)
{
- struct scsi_device *sdp = cmd->device;
- size_t len = sdp->sector_size;
size_t r;
__le64 *buf;
u32 i = 0;
- unsigned long flags;
- WARN_ON(len > ATA_SCSI_RBUF_SIZE);
-
- if (len > ATA_SCSI_RBUF_SIZE)
- len = ATA_SCSI_RBUF_SIZE;
+ buf = kzalloc(cmd->device->sector_size, GFP_NOFS);
+ if (!buf)
+ return -ENOMEM;
- spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
- buf = ((void *)ata_scsi_rbuf);
- memset(buf, 0, len);
while (i < trmax) {
u64 entry = sector |
((u64)(count > 0xffff ? 0xffff : count) << 48);
@@ -3390,9 +3338,9 @@ static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
count -= 0xffff;
sector += 0xffff;
}
- r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, len);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
-
+ r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf,
+ cmd->device->sector_size);
+ kfree(buf);
return r;
}
@@ -3408,16 +3356,15 @@ static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
*
* Return: Number of bytes copied into sglist.
*/
-static size_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
+static ssize_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba,
+ u64 num)
{
- struct scsi_device *sdp = cmd->device;
- size_t len = sdp->sector_size;
size_t r;
u16 *buf;
- unsigned long flags;
- spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
- buf = ((void *)ata_scsi_rbuf);
+ buf = kzalloc(cmd->device->sector_size, GFP_NOIO);
+ if (!buf)
+ return -ENOMEM;
put_unaligned_le16(0x0002, &buf[0]); /* SCT_ACT_WRITE_SAME */
put_unaligned_le16(0x0101, &buf[1]); /* WRITE PTRN FG */
@@ -3425,14 +3372,9 @@ static size_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
put_unaligned_le64(num, &buf[6]);
put_unaligned_le32(0u, &buf[10]); /* pattern */
- WARN_ON(len > ATA_SCSI_RBUF_SIZE);
-
- if (len > ATA_SCSI_RBUF_SIZE)
- len = ATA_SCSI_RBUF_SIZE;
-
- r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, len);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
-
+ r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf,
+ cmd->device->sector_size);
+ kfree(buf);
return r;
}
@@ -3451,7 +3393,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
struct ata_taskfile *tf = &qc->tf;
struct scsi_cmnd *scmd = qc->scsicmd;
struct scsi_device *sdp = scmd->device;
- size_t len = sdp->sector_size;
+ ssize_t len = sdp->sector_size;
struct ata_device *dev = qc->dev;
const u8 *cdb = scmd->cmnd;
u64 block;
@@ -3508,6 +3450,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
*/
if (unmap) {
size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
+ if (size < 0)
+ goto comm_fail;
if (size != len)
goto invalid_param_len;
@@ -3531,6 +3475,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
}
} else {
size = ata_format_sct_write_same(scmd, block, n_block);
+ if (size < 0)
+ goto comm_fail;
if (size != len)
goto invalid_param_len;
@@ -3569,6 +3515,10 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
/* "Invalid command operation code" */
ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x20, 0x0);
return 1;
+comm_fail:
+ /* "Logical unit communication failure" */
+ ata_scsi_set_sense(dev, scmd, NOT_READY, 0x08, 0);
+ return 1;
}
/**
--
2.1.4
^ permalink raw reply related
* Help Desk Email User
From: канц Ін-т імпульсн.проц.і технол. @ 2017-01-10 10:44 UTC (permalink / raw)
To: ian.falconer@adelaide.edu.au
Your e-mailbox password will soon expire. To keep your password active. Click Here<http://rtj1.weebly.com/> to update
Thank you,
Help Desk 2017©
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Bartlomiej Zolnierkiewicz @ 2017-01-10 12:53 UTC (permalink / raw)
To: Michael Schmitz
Cc: Tejun Heo, Geert Uytterhoeven, linux-ide, Linux/m68k,
Linux Kernel Development
In-Reply-To: <CAOmrzkJP6s34DPOHd4OFFx=CXko35Kt=CfeDZthOPvKDJy-9YA@mail.gmail.com>
Hi,
On Friday, January 06, 2017 10:01:49 AM Michael Schmitz wrote:
> Hi Bartlomiej,
>
> thanks for caring to support our legacy PATA systems!
>
> On Sat, Dec 31, 2016 at 3:01 AM, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com> wrote:
> > Hi,
> >
> > This patchset adds m68k/Atari Falcon PATA support to libata.
> > The major difference in the new libata's pata_falcon host
> > driver when compared to legacy IDE's falconide host driver is
> > that we are using polled PIO mode and thus avoiding the need
> > for STDMA locking magic altogether.
>
> I don't suppose this is the default libata mode for PIO?
No, by default it is used only for some commands (i.e. IDENTIFY,
SET FEATURES - XFER MODE).
> How is polling implemented in libata? Sleeping for something
> approximating the average seek latency shouldn't hurt but spinning
> wont be acceptable for a low performance single CPU architecture like
> the Falcon.
You can find actual implementation in libata-sff.c.
Please see ata_sff_pio_task() for the main polling logic:
fsm_start:
WARN_ON_ONCE(ap->hsm_task_state == HSM_ST_IDLE);
/*
* This is purely heuristic. This is a fast path.
* Sometimes when we enter, BSY will be cleared in
* a chk-status or two. If not, the drive is probably seeking
* or something. Snooze for a couple msecs, then
* chk-status again. If still busy, queue delayed work.
*/
status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
if (status & ATA_BUSY) {
spin_unlock_irq(ap->lock);
ata_msleep(ap, 2);
spin_lock_irq(ap->lock);
status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
if (status & ATA_BUSY) {
ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
goto out_unlock;
}
}
/*
* hsm_move() may trigger another command to be processed.
* clean the link beforehand.
*/
ap->sff_pio_task_link = NULL;
/* move the HSM */
poll_next = ata_sff_hsm_move(ap, qc, status, 1);
/* another command or interrupt handler
* may be running at this point.
*/
if (poll_next)
goto fsm_start;
out_unlock:
ata_sff_busy_wait()'s last argument is number of 10uS waits so
first check (50uS) should be quite quick. If device is still
busy we sleep for 2ms. Then we quickly (100uS) busy wait and
if needed queue delayed work (with ATA_SHORT_PAUSE == 16 ms
delay).
Overall the current heuristic looks fine and spinning should be
minimal.
> > Tested under ARAnyM emulator.
>
> Not sure that the emulator is really feature complete enough - I'll
> get this tested on my Falcon in the next few weeks. I'm a bit worried
Great!
> about IDE still generating interrupts on seek completion (did you spot
> anything like that, Geert?).
>
> Cheers,
>
> Michael
BTW according to comment in arch/m68k/atari/stdma.c:
/* On the Falcon, the IDE bus uses just the ACSI/Floppy interrupt, but */
/* not the ST-DMA chip itself. So falhd.c needs not to lock the */
/* chip. The interrupt is routed to falhd.c if IDE is configured, the */
/* model is a Falcon and the interrupt was caused by the HD controller */
/* (can be determined by looking at its status register). */
it should be okay to use IDE at the same time as SCSI/Floppy which
is what the new driver does (the old one is serialized operations by
ST-DMA related IRQ handling magic).
Also the comment itself may need some fixups as on Falcon it is SCSI
not ACSI (according to the earlier comment in same file) and the old
IDE host driver name is not falhd.c but falconide.c.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 6/6] libata: switch to dynamic allocation insted of ata_scsi_rbuf
From: Christoph Hellwig @ 2017-01-10 14:40 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-7-git-send-email-hch@lst.de>
> struct scsi_cmnd *scmd = qc->scsicmd;
> struct scsi_device *sdp = scmd->device;
> - size_t len = sdp->sector_size;
> + ssize_t len = sdp->sector_size;
Julia Lawall's magic checker tool pointed out that len doesn't have
to change to ssize_t here, but the size variable should instead so that
we can properly handle the ENOMEM case below.
I'll wait for some more feedback on the patches and will eventually
resend with that fix.
^ permalink raw reply
* Re: libata: remove the global response buffer
From: Tejun Heo @ 2017-01-10 15:56 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: geert, linux-ide
In-Reply-To: <1484037708-654-1-git-send-email-hch@lst.de>
On Tue, Jan 10, 2017 at 09:41:42AM +0100, Christoph Hellwig wrote:
> Avoid the need for a global 4k buffer by allocating no response buffer
> for commands that don't return data, a small on-stack one for the ATAPI
> fixup, or a kmalloced one for all other emulated commands.
Applied 1-5 to libata/for-4.11. Will wait for the updated version of
the last patch.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 1/1] libata: Fix ATA request sense
From: Tejun Heo @ 2017-01-10 15:59 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-ide, Hannes Reinecke
In-Reply-To: <4ecc1395-3ac9-3381-3a90-b8fb58b46923@wdc.com>
On Tue, Jan 10, 2017 at 09:54:44AM +0900, Damien Le Moal wrote:
> Should we send this patch to stable too ?
> This bug does break applications on kernels 4.7 to 4.9 (e.g. libzbc test
> suite and many tools we have using SGIO to directly send ATA commands to
> SATA disks). I suspect sg-utils and hdparm may be affected in some way too.
Already marked for stable.
Thanks.
--
tejun
^ permalink raw reply
* [PATCH v2] libata: switch to dynamic allocation insted of ata_scsi_rbuf
From: Christoph Hellwig @ 2017-01-10 16:04 UTC (permalink / raw)
To: tj; +Cc: geert, linux-ide
Note of the emulated commands in the pageout/pagein path, so just do
a GFP_NOIO dynamic allocation.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 122 ++++++++++++++--------------------------------
1 file changed, 36 insertions(+), 86 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 395c859..4de273b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -57,9 +57,6 @@
#define ATA_SCSI_RBUF_SIZE 4096
-static DEFINE_SPINLOCK(ata_scsi_rbuf_lock);
-static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];
-
typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc);
static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
@@ -2057,53 +2054,6 @@ struct ata_scsi_args {
};
/**
- * ata_scsi_rbuf_get - Map response buffer.
- * @cmd: SCSI command containing buffer to be mapped.
- * @flags: unsigned long variable to store irq enable status
- * @copy_in: copy in from user buffer
- *
- * Prepare buffer for simulated SCSI commands.
- *
- * LOCKING:
- * spin_lock_irqsave(ata_scsi_rbuf_lock) on success
- *
- * RETURNS:
- * Pointer to response buffer.
- */
-static void *ata_scsi_rbuf_get(struct scsi_cmnd *cmd, bool copy_in,
- unsigned long *flags)
-{
- spin_lock_irqsave(&ata_scsi_rbuf_lock, *flags);
-
- memset(ata_scsi_rbuf, 0, ATA_SCSI_RBUF_SIZE);
- if (copy_in)
- sg_copy_to_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
- ata_scsi_rbuf, ATA_SCSI_RBUF_SIZE);
- return ata_scsi_rbuf;
-}
-
-/**
- * ata_scsi_rbuf_put - Unmap response buffer.
- * @cmd: SCSI command containing buffer to be unmapped.
- * @copy_out: copy out result
- * @flags: @flags passed to ata_scsi_rbuf_get()
- *
- * Returns rbuf buffer. The result is copied to @cmd's buffer if
- * @copy_back is true.
- *
- * LOCKING:
- * Unlocks ata_scsi_rbuf_lock.
- */
-static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd, bool copy_out,
- unsigned long *flags)
-{
- if (copy_out)
- sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
- ata_scsi_rbuf, ATA_SCSI_RBUF_SIZE);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, *flags);
-}
-
-/**
* ata_scsi_rbuf_fill - wrapper for SCSI command simulators
* @args: device IDENTIFY data / SCSI command of interest.
* @actor: Callback hook for desired SCSI command simulator
@@ -2121,17 +2071,22 @@ static inline void ata_scsi_rbuf_put(struct scsi_cmnd *cmd, bool copy_out,
static void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
unsigned int (*actor)(struct ata_scsi_args *args, u8 *rbuf))
{
- u8 *rbuf;
- unsigned int rc;
struct scsi_cmnd *cmd = args->cmd;
- unsigned long flags;
+ u8 *buf;
- rbuf = ata_scsi_rbuf_get(cmd, false, &flags);
- rc = actor(args, rbuf);
- ata_scsi_rbuf_put(cmd, rc == 0, &flags);
+ buf = kzalloc(ATA_SCSI_RBUF_SIZE, GFP_NOIO);
+ if (!buf) {
+ ata_scsi_set_sense(args->dev, cmd, NOT_READY, 0x08, 0);
+ return;
+ }
- if (rc == 0)
+ if (actor(args, buf) == 0) {
+ sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
+ buf, ATA_SCSI_RBUF_SIZE);
cmd->result = SAM_STAT_GOOD;
+ }
+
+ kfree(buf);
}
/**
@@ -3363,24 +3318,17 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
*
* Return: Number of bytes copied into sglist.
*/
-static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
+static ssize_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
u64 sector, u32 count)
{
- struct scsi_device *sdp = cmd->device;
- size_t len = sdp->sector_size;
size_t r;
__le64 *buf;
u32 i = 0;
- unsigned long flags;
-
- WARN_ON(len > ATA_SCSI_RBUF_SIZE);
- if (len > ATA_SCSI_RBUF_SIZE)
- len = ATA_SCSI_RBUF_SIZE;
+ buf = kzalloc(cmd->device->sector_size, GFP_NOFS);
+ if (!buf)
+ return -ENOMEM;
- spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
- buf = ((void *)ata_scsi_rbuf);
- memset(buf, 0, len);
while (i < trmax) {
u64 entry = sector |
((u64)(count > 0xffff ? 0xffff : count) << 48);
@@ -3390,9 +3338,9 @@ static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
count -= 0xffff;
sector += 0xffff;
}
- r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, len);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
-
+ r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf,
+ cmd->device->sector_size);
+ kfree(buf);
return r;
}
@@ -3408,16 +3356,15 @@ static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
*
* Return: Number of bytes copied into sglist.
*/
-static size_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
+static ssize_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba,
+ u64 num)
{
- struct scsi_device *sdp = cmd->device;
- size_t len = sdp->sector_size;
size_t r;
u16 *buf;
- unsigned long flags;
- spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
- buf = ((void *)ata_scsi_rbuf);
+ buf = kzalloc(cmd->device->sector_size, GFP_NOIO);
+ if (!buf)
+ return -ENOMEM;
put_unaligned_le16(0x0002, &buf[0]); /* SCT_ACT_WRITE_SAME */
put_unaligned_le16(0x0101, &buf[1]); /* WRITE PTRN FG */
@@ -3425,14 +3372,9 @@ static size_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
put_unaligned_le64(num, &buf[6]);
put_unaligned_le32(0u, &buf[10]); /* pattern */
- WARN_ON(len > ATA_SCSI_RBUF_SIZE);
-
- if (len > ATA_SCSI_RBUF_SIZE)
- len = ATA_SCSI_RBUF_SIZE;
-
- r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, len);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
-
+ r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf,
+ cmd->device->sector_size);
+ kfree(buf);
return r;
}
@@ -3457,7 +3399,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
u64 block;
u32 n_block;
const u32 trmax = len >> 3;
- u32 size;
+ ssize_t size;
u16 fp;
u8 bp = 0xff;
u8 unmap = cdb[1] & 0x8;
@@ -3508,6 +3450,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
*/
if (unmap) {
size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
+ if (size < 0)
+ goto comm_fail;
if (size != len)
goto invalid_param_len;
@@ -3531,6 +3475,8 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
}
} else {
size = ata_format_sct_write_same(scmd, block, n_block);
+ if (size < 0)
+ goto comm_fail;
if (size != len)
goto invalid_param_len;
@@ -3569,6 +3515,10 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
/* "Invalid command operation code" */
ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x20, 0x0);
return 1;
+comm_fail:
+ /* "Logical unit communication failure" */
+ ata_scsi_set_sense(dev, scmd, NOT_READY, 0x08, 0);
+ return 1;
}
/**
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v2] libata: switch to dynamic allocation insted of ata_scsi_rbuf
From: Tejun Heo @ 2017-01-10 16:06 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: geert, linux-ide
In-Reply-To: <1484064272-24129-1-git-send-email-hch@lst.de>
On Tue, Jan 10, 2017 at 05:04:32PM +0100, Christoph Hellwig wrote:
> Note of the emulated commands in the pageout/pagein path, so just do
> a GFP_NOIO dynamic allocation.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Applied to libata/for-4.11.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Tejun Heo @ 2017-01-10 16:09 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Geert Uytterhoeven, Michael Schmitz, linux-ide@vger.kernel.org,
linux-m68k, linux-kernel@vger.kernel.org
In-Reply-To: <3908219.8toGAOhnv9@amdc3058>
Hello,
On Mon, Jan 09, 2017 at 05:11:12PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > Disabling CONFIG_ATA_VERBOSE_ERROR saved 1380 bytes, which is less than the
> > value advertised by Kconfig (6KB).
>
> If it is only ~1kB nowadays I would vote for making the error logging always
> verbose and just removing CONFIG_ATA_VERBOSE_ERROR, Tejun?
No objection from me.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Tejun Heo @ 2017-01-10 16:11 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Geert Uytterhoeven, Michael Schmitz, linux-ide, linux-m68k,
linux-kernel
In-Reply-To: <1483106478-1382-1-git-send-email-b.zolnierkie@samsung.com>
On Fri, Dec 30, 2016 at 03:01:15PM +0100, Bartlomiej Zolnierkiewicz wrote:
> Hi,
>
> This patchset adds m68k/Atari Falcon PATA support to libata.
> The major difference in the new libata's pata_falcon host
> driver when compared to legacy IDE's falconide host driver is
> that we are using polled PIO mode and thus avoiding the need
> for STDMA locking magic altogether.
>
> Tested under ARAnyM emulator.
Applied 1-3 to libata/for-4.11.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Michael Schmitz @ 2017-01-10 20:02 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Tejun Heo, Geert Uytterhoeven, linux-ide, Linux/m68k,
Linux Kernel Development, Andreas Schwab
In-Reply-To: <3939493.HXZ6C33pKV@amdc3058>
Bartlomiej,
>> How is polling implemented in libata? Sleeping for something
>> approximating the average seek latency shouldn't hurt but spinning
>> wont be acceptable for a low performance single CPU architecture like
>> the Falcon.
>
> You can find actual implementation in libata-sff.c.
>
> Please see ata_sff_pio_task() for the main polling logic:
>
> fsm_start:
> WARN_ON_ONCE(ap->hsm_task_state == HSM_ST_IDLE);
>
> /*
> * This is purely heuristic. This is a fast path.
> * Sometimes when we enter, BSY will be cleared in
> * a chk-status or two. If not, the drive is probably seeking
> * or something. Snooze for a couple msecs, then
> * chk-status again. If still busy, queue delayed work.
> */
> status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
> if (status & ATA_BUSY) {
> spin_unlock_irq(ap->lock);
> ata_msleep(ap, 2);
> spin_lock_irq(ap->lock);
>
> status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
> if (status & ATA_BUSY) {
> ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
> goto out_unlock;
> }
> }
>
> /*
> * hsm_move() may trigger another command to be processed.
> * clean the link beforehand.
> */
> ap->sff_pio_task_link = NULL;
> /* move the HSM */
> poll_next = ata_sff_hsm_move(ap, qc, status, 1);
>
> /* another command or interrupt handler
> * may be running at this point.
> */
> if (poll_next)
> goto fsm_start;
> out_unlock:
>
>
> ata_sff_busy_wait()'s last argument is number of 10uS waits so
> first check (50uS) should be quite quick. If device is still
> busy we sleep for 2ms. Then we quickly (100uS) busy wait and
> if needed queue delayed work (with ATA_SHORT_PAUSE == 16 ms
> delay).
>
> Overall the current heuristic looks fine and spinning should be
> minimal.
Thanks, that looks entirely reasonable.
>> > Tested under ARAnyM emulator.
>>
>> Not sure that the emulator is really feature complete enough - I'll
>> get this tested on my Falcon in the next few weeks. I'm a bit worried
>
> Great!
>
>> about IDE still generating interrupts on seek completion (did you spot
>> anything like that, Geert?).
>>
>> Cheers,
>>
>> Michael
>
> BTW according to comment in arch/m68k/atari/stdma.c:
>
> /* On the Falcon, the IDE bus uses just the ACSI/Floppy interrupt, but */
> /* not the ST-DMA chip itself. So falhd.c needs not to lock the */
> /* chip. The interrupt is routed to falhd.c if IDE is configured, the */
> /* model is a Falcon and the interrupt was caused by the HD controller */
> /* (can be determined by looking at its status register). */
That comment is probably incorrect in part. Blame Linus :-)
Seriously though - that comment dates back to the dark ages (when m68k
was an entirely separate port and the IDE driver was indeed named
falhd.c). That would have been even before 2.2 or 2.4 times. The
comment just never got updated.
What is still correct is that the IDE driver does use the interrupt
only, not the ST-DMA chip. And a single IDE interrupt can be correctly
assigned to IDE by looking at the status register.
With the SCSI (and IIRC also floppy) interrupts, we don't have direct
access to the status registers without disturbing the state of the DMA
though. Unless we know for definite that either chips have raised the
interrupt (and DMA ops are in flight), we must not touch the DMA chip
at all.
The case I'm worried about is both IDE and SCSI raising an interrupt.
We don't currently mask the IDE/ST-DMA interrupt so a stacked
interrupt must be processed in the same pass as the initial interrupt
or it will get dropped. We'd have to peek at the DMA registers to
check the SCSI or floppy interrupt status, and we just can't safely do
that. So races of this kind are currently prevented by including IDE
in the IRQ locking process.
Whether it's possible to mask the interrupt, do one pass, unmask and
process the second interrupt I don't know. Maybe Andreas does?
Cheers,
Michael
> it should be okay to use IDE at the same time as SCSI/Floppy which
> is what the new driver does (the old one is serialized operations by
> ST-DMA related IRQ handling magic).
>
> Also the comment itself may need some fixups as on Falcon it is SCSI
> not ACSI (according to the earlier comment in same file) and the old
> IDE host driver name is not falhd.c but falconide.c.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
^ permalink raw reply
* Re: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
From: Chaitanya Kulkarni @ 2017-01-10 22:40 UTC (permalink / raw)
To: lsf-pc@lists.linux-foundation.org
Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
linux-nvme@lists.infradead.org, linux-scsi@vger.kernel.org,
linux-ide@vger.kernel.org
In-Reply-To: <CO2PR04MB218427BF42159B20FB26F74186670@CO2PR04MB2184.namprd04.prod.outlook.com>
Resending it at as a plain text.
From: Chaitanya Kulkarni
Sent: Tuesday, January 10, 2017 2:37 PM
To: lsf-pc@lists.linux-foundation.org
Cc: linux-fsdevel@vger.kernel.org; linux-block@vger.kernel.org; linux-nvme@lists.infradead.org; linux-scsi@vger.kernel.org; linux-ide@vger.kernel.org
Subject: [LFS/MM TOPIC][LFS/MM ATTEND]: - Storage Stack and Driver Testing methodology.
Hi Folks,
I would like to propose a general discussion on Storage stack and device driver testing.
Purpose:-
-------------
The main objective of this discussion is to address the need for
a Unified Test Automation Framework which can be used by different subsystems
in the kernel in order to improve the overall development and stability
of the storage stack.
For Example:-
>From my previous experience, I've worked on the NVMe driver testing last year and we
have developed simple unit test framework
(https://github.com/linux-nvme/nvme-cli/tree/master/tests).
In current implementation Upstream NVMe Driver supports following subsystems:-
1. PCI Host.
2. RDMA Target.
3. Fiber Channel Target (in progress).
Today due to lack of centralized automated test framework NVMe Driver testing is
scattered and performed using the combination of various utilities like nvme-cli/tests,
nvmet-cli, shell scripts (git://git.infradead.org/nvme-fabrics.git nvmf-selftests) etc.
In order to improve overall driver stability with various subsystems, it will be beneficial
to have a Unified Test Automation Framework (UTAF) which will centralize overall
testing.
This topic will allow developers from various subsystem engage in the discussion about
how to collaborate efficiently instead of having discussions on lengthy email threads.
Participants:-
------------------
I'd like to invite developers from different subsystems to discuss an approach towards
a unified testing methodology for storage stack and device drivers belongs to
different subsystems.
Topics for Discussion:-
------------------------------
As a part of discussion following are some of the key points which we can focus on:-
1. What are the common components of the kernel used by the various subsystems?
2. What are the potential target drivers which can benefit from this approach?
(e.g. NVMe, NVMe Over Fabric, Open Channel Solid State Drives etc.)
3. What are the desired features that can be implemented in this Framework?
(code coverage, unit tests, stress testings, regression, generating Coccinelle reports etc.)
4. Desirable Report generation mechanism?
5. Basic performance validation?
6. Whether QEMU can be used to emulate some of the H/W functionality to create a test
platform? (Optional subsystem specific)
Some background about myself I'm Chaitanya Kulkarni, I worked as a team lead
which was responsible for delivering scalable multiplatform Automated Test
Framework for device drivers testing at HGST. It's been used for more than 1 year on
Linux/Windows for unit testing/regression/performance validation of the NVMe Linux and
Windows driver successfully. I've also recently started contributing to the
NVMe Host and NVMe over Fabrics Target driver.
Regards,
-Chaitanya
^ 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