Re: new ata_port_operations for .pmp_{read,write} ?
Hello, Mark. Mark Lord wrote: >> libata doesn't really put much restrictions on what a LLD should do on >> entering EH and if the controller's behavior is predictable, there's no >> reason to freeze the port. If the problem is that the DMA engine isn't >> usable after PMP error but it's known that the controller isn't gonna >> cause irq storm, no need to freeze. The only command EH issues before >> resetting which can use DMA protocol is READ_LOG_EXT. Maybe there needs >> to be a way to force PIO protocol for READ_LOG_EXT. Other than that, if >> no-data and PIO-only commands work fine, EH autopsy should work fine. > .. > > Eh? READ LOG EXT *is* a PIO command, as opposed to READ LOG DMA EXT > which uses DMA. Ah.. right. My bad. > And the EH also issues PIO READ_BUFFER commands for PM ports, if a PM is > present. H Where? -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ops->qc_defer not invoked on ata_exec_internal_sg() paths ?
Hello, Mark. Mark Lord wrote: > The optional .qc_defer() methods don't seem to be called > on the ata_exec_internal_sg() path. > > At present, this is probably okay. But in the future, > as we add functionality for link power management > and hotplug polling, this could be a problem. > > I think. Or is it possibly also a problem today > for sata_send_pmp() and friends ? That's intentional and okay. Currently, EH can only issue one non-NCQ command at a time and dynamic link PM and hotplug polling shouldn't be done via EH anyway. EH is way too big a hammer for those. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new ata_port_operations for .pmp_{read,write} ?
Hello, Mark. Mark Lord wrote: > MMmm.. maybe the "vendor unique FIS" mechanism of the chipset > can save the scenario here. It would seem to be a reasonable > way to direct a FIS (anything up to 8KB) at a specific pmp, > without changing the "default" pmp on the channel. > > I can have qc_issue use that mechanism for anything destined > for pmp==15. If it works. The Marvell proprietary driver > has some kludgey status polling wrapped around their own use of it. > > One of the chip errata apparently requires this anyway for doing > the READ_LOG_EXT commands after a device error, so perhaps it will > work out to be useful in more ways. Hmmm... > Speaking of which.. I would like to sort out the "freeze" stuff, > so that the sata_mv EH doesn't lock out the PMP commands as it > does today. > > Can you recap what a LLD should be doing in the presence of a PM > for EH purposes? Eg. media error on a PMP drive, so what core > ATA functions should the sata_mv EH interrupt handler be calling, > and in what sequence.. so that the libata-pmp/eh code can still > succeed in it's queries to the PM? libata doesn't really put much restrictions on what a LLD should do on entering EH and if the controller's behavior is predictable, there's no reason to freeze the port. If the problem is that the DMA engine isn't usable after PMP error but it's known that the controller isn't gonna cause irq storm, no need to freeze. The only command EH issues before resetting which can use DMA protocol is READ_LOG_EXT. Maybe there needs to be a way to force PIO protocol for READ_LOG_EXT. Other than that, if no-data and PIO-only commands work fine, EH autopsy should work fine. > Like I noted before, sata_mv will need to eventually hard reset > the channel regardless, but it does seem permitted to use PIO > or vendor-unique-FIS PIO (with polling for either) in the interim. Just set ATA_EH_HARDRESET (which will soon become ATA_EH_RESET with prefer-COMRESET patchset) from the interrupt handler before requesting EH. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new ata_port_operations for .pmp_{read,write} ?
Mark Lord wrote: > No, the quickest solution for sata_mv, the one I apparently will now be > using, > is to just clone about 250 lines of reset/debouce/probe code from > libata-core > and change perhaps five lines of it to work around this issue plus some > chipset errata. > > What I was thinking of, before, is the SATA specification which details > use of bits in the SCR to select a PMP port. Pretty much exactly as > this chipset does it, except in the SCR rather than a proprieary port. > > But no big deal. I can clone code and not bother you any more. > In fact, some of the cloned code was already in sata_mv, and I removed > it this past week in my local working copy. I'll just restore that, > along with another big blob so that we can select pm port where needed. > > What a shame. The order is somewhat reversed here and I can understand why you're frustrated but I'm just trying to make things look right in long term, so feel free to bother me. :-) For temporary solution, I'm okay either way. I'll clean things up later when the necessary core changes are made. * Adding ->pmp_select or ->scr_pmp_rw or whatever callback to work around the current problem. * Duplicate code in sata_mv. Whichever way you go, can you please mark where it's different and why? Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new ata_port_operations for .pmp_{read,write} ?
Hello, Mark.
Mark Lord wrote:
> Mark Lord wrote:
>> An alternative to all this, might be to expose the "select_pmp()"
>> function shown in the sample code, and have libata-pmp.c call that,
>> instead of having the new new .pmp_{read,write} functions.
> ..
>
> I wonder if this might be more viable than first thought.
>
> Say the LLD, be it ata_piix or sata_mv or sata_svw, were to provide
> an option ata_operations method for "select_pmp_port(pmp)",
> which the core could invoke prior to any direct manipulation
> of the shadow registers.
I don't really think that's a good solution. That's the quickest
solution for sata_mv but it just works around more fundamental problem
of assuming SFF behavior in core layer which we need to drop anyway.
> I really would like to keep the LLD code small, and have good solid
> core routines for non-hardware specific functionality. All of this stuff
> I'm beginning to do with sata_mv would be trivial if I wanted to bloat
> the LLD, but really.. only a tiny bit of it need be custom to sata_mv.
>
> The existing SFF reset/probe/pmp stuff is just about exactly what
> sata_mv needs.. and I feel a strong desire to not clone/duplicate
> that hard-won functionality.
I strongly agree but am having difficult time agreeing with the proposed
approach.
> Much of it I can see being shared with other half-and-half chipset drivers
> as we add PMP functionality to those.
Do we have other chips which have PMP support but still uses mixed SFF
interface?
> Maybe that's just the embedded side me showing through?
>
> It is tricky to define the right interfaces, though, as each chipset
> does throw its own unique curve balls at us.
Yeah, exactly. I think what needs to be done is to separate out SFF
assumptions from core layer, factor out SFF-proper helpers and use them
to implement LLDs for quasi-SFF controllers.
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new ata_port_operations for .pmp_{read,write} ?
Hello, Mark. Mark Lord wrote: >>> And for that matter, is it possible for sata_pmp_read() to be called >>> while the link is active with another command ? Not today, it seems, >>> but what about when hotplug polling gets implemented ? >> .. >> That's the one I'm most concerned about. Should I be? > ... > > Tejun, > > On a related note, I'm now looking into PMP error handling in the driver. > The obvious thing I see that I want to fix, is that after a media error > on any PMP attached drive, I get this: > > ata20.00: failed to read SCR 1 (Emask=0x40) > ata20.01: failed to read SCR 1 (Emask=0x40) > ata20.02: failed to read SCR 1 (Emask=0x40) > ata20.03: failed to read SCR 1 (Emask=0x40) > ata20.04: failed to read SCR 1 (Emask=0x40) > > Okay, so those are from sata_pmp_read(), which cannot even > issue it's commands because the port was frozen by the EH. Hmm.. media error causes freeze? Anyways, yeah, those are expected if the port is frozen. Those are from link autopsy. Maybe it's better to skip SCR access on fan-out ports if host port is frozen or at least suppress the messages. > Is this expected? I'm not entirely clear what to do in > the EH for this driver. The chipset docs say that > after just about any kind of error software must do > a hard reset of the channel to make it usable again. > > But I suspect that PIO commands may be okay before that, > and sata_pmp_read() is trying to issue a PIO command. If the controller needs to be frozen after any kind of error, I don't think there's much left to do other than suppressing those annoying messages. Hmmm.. how does the controller handle ATAPI check sense? -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: new ata_port_operations for .pmp_{read,write} ?
Hello, Mark. Mark Lord wrote: >> This patch provides two new struct ata_port_operations methods for this, >> and modifies the code in libata-pmp to use them if provided. > ... > > Note that, while this does work for sata_mv, I'm still thinking about it. > > I'm not totally clear yet (more reading to do) as to how/when > the ATA shadow/taskfile registers get updated to reflect those > for the currently selected pmp.. > > It would seem that with other parts of libata-sff directly reading > from the ctl, status, and altstatus "registers" during polling, > command setup, and probing, that there might (?) be a loophole > somewhere in this strategy. Yeah, this is an interesting problem. There are basically multiple sets of TF registers and the SFF way of assuming single set doesn't really work well and I don't really think adding ->pmp_read/write is the correct long term solution to the problem. We need to confine direct TF register accesses to SFF layer only as controllers which don't implement SFF interface may or may not emulate TF registers and even when they do, it sometimes can't really match the SFF behavior. For command execution, TF write is already performed by qc_prep and issue and TF read back should be performed by LLD if RESULT_TF is set. For EH, the reset methods should be responsible whether or how the TF registers are accessed. Mark, for your case, I think the correct thing to do is to factor out the necessary bare-bone part from SFF implementation and use it with necessary PMP register setup once the necessary change is made to the core layer. The controller is NOT a SFF one and I don't think stretching SFF implementation to fit mv's PMP-aware emulation of it is a good idea. Maybe we can fit libata to it but it's likely we'll need to modify it again to fit different emulation from different vendor. > Is this scenario going to be possible: somebody calls sata_pmp_read() > as part of, say, hotplug polling, and after that operation completes > we then have code that calls ata_check_status() prior to the next > tf_load / command issue ? If so, they'll see the wrong cached shadow > status register. I think what should happen is that any of TF registers isn't accessed behind LLD's back. Then, you don't have nothing to worry about. If the controller emulates some of SFF interface, you can always properly wrap SFF helpers with necessary setup and teardown added. > Mmm.. an amazing amount of complexity there for something that > ought to be very simple. I wish things are a bit clearer now. I think the problem here is that we're assuming SFF TF access on controllers which aren't really SFF. For sil24 and ahci, the driver emulates it and it isn't too difficult. The picture gets more interesting for sata_mv as its hardware interface much closer to SFF than sil24 or ahci and TF registers matter much more. For ahci and sil24, LLD can just fool libata core layer which assumes ubiquitous TF access. TF registers don't really matter to controller operation anyway and feeding bogus values work well. For sata_mv, it's different. Those registers are integral part of controller operation and sata_mv can't really tolerate core layer stepping in w/o notifying LLD. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] AMD SB700/SB800 SATA support 64bit DMA
Shane Huang wrote: > Jeff and Tejun: > > > I'm using my private mailbox to submit it before I set up > the connection to our exchange mail server under linux. > Please check whether this patch can be accepted. Patch looks fine to me. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] AMD SB700/SB800 SATA support 64bit DMA
Huang, Shane wrote: > Jeff: > > SB700 SATA controller can support 64 bit DMA, the previous commit > badc2341579511a247f5993865aa68379e283c5c was added with > careless reference to SB600, which should be modified by this patch. > > > Signed-off-by: Shane Huang <[EMAIL PROTECTED]> Shane, it still wraps. Just give up outlook and use thunderbird + toggle wordwrap + external editor or mutt/pine. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's needed for PMP support?
Mark Lord wrote: >> Heh... I never thought a PMP aware controller would use TF SRST, so what >> you want to do is set pmp value in the register and calling >> ata_std_softreset(), right? I think the correct thing to do is to >> separate out SRST sequence proper from ata_std_softreset() into, say, >> ata_sff_SRST() and build custom softreset around it. After all, the >> problem here is the reset sequence not the SCR access. > .. > > Actually, I believe the problem *is* the (pmp) SCR access. > The same issue will return again when trying to support hotplug, for > example. Can you elaborate a bit? > Any SCR access will steal the active pmp on such hosts. > > I think we really do need to snoop those, somehow. Adding ->sata_pmp_scr_read/write should do but I wanna avoid that if possible. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMicron - hard resetting link
Hello, Gabor FUNK wrote: > To sum it up: > - 1st the 4 disks on the Jmicron controller failed with 1 [chieftek] PSU > - then it failed with 2 PSU too, but this time the chieftek was only > connected to the different 4 disks - on the nvidia controller. MB > and other disks were on the other, non-chieftek [650W] PSU. > - Then I started the tests with only this second PSU, and it ran > for about 6 days under heavy testing and array rebuilding and > guess what: it failed again. Eeekk.. > Full kernel log at: > http://www.huweb.hu/maques/tmp/jmicron/kern0221.log > > Since it is not a "switch on and see" problem, I'm not in too good > position, so unless someone have a really great idea or observation, > I seriously have to consider to replace the MB and probably add > some extra sata controllers. If you can still do some testing, what happens if you unplug power to the failed drive and replug it while the system is still running? Does hotplug event get triggered and the drive gets recognized again? If so, does unplugging and replugging the SATA controller only (w/o powering down the drive) achieve the same thing? Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's needed for PMP support?
Mark Lord wrote:
> Mark Lord wrote:
>> Tejun Heo wrote:
>>>>
>>>> The following things are needed for a LLD to support PMP.
>>> ..
>>>> I think that's about it. Feel free to ask if something isn't clear.
>> ..
>>
>> I think we need better semantics around sata_scr_{read,write}(),
>> or more specifically
>> These need to be moved into ata_port_operations
>> so that LLDs can wrap them to properly manage
>> the host controller's global link->pmp value.
> ..
>
> Heck, if .dev_select() took a *device* instead of a *port*
> as it's parameter, then I could probably manage it fine in there.
Heh... I never thought a PMP aware controller would use TF SRST, so what
you want to do is set pmp value in the register and calling
ata_std_softreset(), right? I think the correct thing to do is to
separate out SRST sequence proper from ata_std_softreset() into, say,
ata_sff_SRST() and build custom softreset around it. After all, the
problem here is the reset sequence not the SCR access.
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata-pmp: clear hob for pmp register accesses
Mark Lord wrote: > Mark Lord wrote: >> Tejun Heo wrote: >>> Hello, Mark. >>> >>> Mark Lord wrote: >>>> Tejun, I've added PMP to sata_mv, and am now trying to get it >>>> to work with a Marvell PM attached. >>>> >>>> And the behaviour I see is very bizarre. >>>> >>>> After hard+soft resets, the PM signature is found, >>>> and libata interrogates the PM registers. >>>> >>>> It successfully reads register 0, and then register 1. >>>> But all subsequent registers read out (incorrectly) as zeros. > .. > > Saeed has confirmed this behaviour with a SATA analyzer. > The Marvell port-multiplier apparently likes to see clean HOB > information when accessing PMP registers. > > Since sata_mv uses PIO shadow register access, this doesn't happen > automatically, as it might in a more purely FIS-based driver (eg. ahci). > > One way to fix this is to flag these commands with ATA_TFLAG_LBA48, > forcing libata to write out the HOB fields with known (zero) values. > > Signed-off-by: Saeed Bishara <[EMAIL PROTECTED]> > Acked-by: Mark Lord <[EMAIL PROTECTED]> Acked-by: Tejun Heo <[EMAIL PROTECTED]> I think this is correct w/ or w/o the mv problem. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: Register for dock events when the drive is inside a dock station
> If a device/bay is inside a docking station, we need to register for dock
> events additionally to bay events. If a dock event occurs, the dock driver
> will call the appropriate handler (ata_acpi_ap_notify() or
> ata_acpi_dev_notify()) for us.
>
> Signed-off-by: Holger Macht <[EMAIL PROTECTED]>
> ---
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 9e8ec19..563ad72 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -191,20 +191,33 @@ void ata_acpi_associate(struct ata_host *host)
> else
> ata_acpi_associate_ide_port(ap);
>
> - if (ap->acpi_handle)
> + if (ap->acpi_handle) {
> acpi_install_notify_handler (ap->acpi_handle,
>ACPI_SYSTEM_NOTIFY,
>ata_acpi_ap_notify,
>ap);
> +#ifdef CONFIG_ACPI_DOCK_MODULE
Heh, you need
#if defined(CONFIG_ACPI_DOCK) || defined(CONFIG_ACPI_DOCK_MODULE)
Also, another question. Is there a way to tell whether the device or
port is connected behind a dock or not? Just notifying hotplug signal
is fine for hotplugging but to make hot unplug safe for PATA, libata
should be able to tell whether the device is actually gonna go away and
kill it explicitly.
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sata_sil24 not configuring drive right?
Tejun Heo wrote: >>>> LBA, IORDY(can be disabled) >>>> Queue depth: 1 > > Hmm... Here it is. Interesting. The drive is reporting queue depth of > 1. Interesting. This is the first time I see this. The drive is > telling libata that it supports NCQ and the max queue depth is 1 and > libata configures it accordingly. I'll ask wd about it. Okay, confirmed. The drive does support NCQ but have queue depth of 1, so nothing's wrong with your setup. That's how the drive is built. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libata: automatically use DMADIR if drive/bridge requires it
Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.
That's nice, but most SATA devices which need it will tell us about it
in their IDENTIFY PACKET response, as bit-15 of word-62 of the
returned data (as per ATA7, ATA8 specifications).
So for those which specify it, we should automatically use the DMADIR bit.
Otherwise, disc writing will fail by default on many SATA-ATAPI drives.
This patch adds ATA_DFLAG_DMADIR and make ata_dev_configure() set it
if atapi_dmadir is set or identify data indicates DMADIR is necessary.
atapi_xlat() is converted to check ATA_DFLAG_DMADIR before setting
DMADIR.
Original patch is from Mark Lord.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: Mark Lord <[EMAIL PROTECTED]>
---
I don't have a bridge which sets DMADIR but so only checked atapi_dmadir
parameter. Thanks.
drivers/ata/libata-core.c | 11 +--
drivers/ata/libata-scsi.c |3 ++-
include/linux/ata.h |5 +
include/linux/libata.h|1 +
4 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3011919..d38f113 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2200,6 +2200,7 @@ int ata_dev_configure(struct ata_device *dev)
else if (dev->class == ATA_DEV_ATAPI) {
const char *cdb_intr_string = "";
const char *atapi_an_string = "";
+ const char *dma_dir_string = "";
u32 sntf;
rc = atapi_cdb_len(id);
@@ -2240,13 +2241,19 @@ int ata_dev_configure(struct ata_device *dev)
cdb_intr_string = ", CDB intr";
}
+ if (atapi_dmadir || atapi_id_dmadir(dev->id)) {
+ dev->flags |= ATA_DFLAG_DMADIR;
+ dma_dir_string = ", DMADIR";
+ }
+
/* print device info to dmesg */
if (ata_msg_drv(ap) && print_info)
ata_dev_printk(dev, KERN_INFO,
- "ATAPI: %s, %s, max %s%s%s\n",
+ "ATAPI: %s, %s, max %s%s%s%s\n",
modelbuf, fwrevbuf,
ata_mode_string(xfer_mask),
- cdb_intr_string, atapi_an_string);
+ cdb_intr_string, atapi_an_string,
+ dma_dir_string);
}
/* determine max_sectors */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index c02c490..f50b96b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2543,7 +2543,8 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
qc->tf.protocol = ATAPI_PROT_DMA;
qc->tf.feature |= ATAPI_PKT_DMA;
- if (atapi_dmadir && (scmd->sc_data_direction != DMA_TO_DEVICE))
+ if ((dev->flags & ATA_DFLAG_DMADIR) &&
+ (scmd->sc_data_direction != DMA_TO_DEVICE))
/* some SATA bridges need us to indicate data xfer
direction */
qc->tf.feature |= ATAPI_DMADIR;
}
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 78bbaca..1c622e2 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -659,6 +659,11 @@ static inline int atapi_command_packet_set(const u16
*dev_id)
return (dev_id[0] >> 8) & 0x1f;
}
+static inline int atapi_id_dmadir(const u16 *dev_id)
+{
+ return ata_id_major_version(dev_id) >= 7 && (dev_id[62] & 0x8000);
+}
+
static inline int is_multi_taskfile(struct ata_taskfile *tf)
{
return (tf->command == ATA_CMD_READ_MULTI) ||
diff --git a/include/linux/libata.h b/include/linux/libata.h
index bc5a8d0..b942e90 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -138,6 +138,7 @@ enum {
ATA_DFLAG_AN= (1 << 7), /* AN configured */
ATA_DFLAG_HIPM = (1 << 8), /* device supports HIPM */
ATA_DFLAG_DIPM = (1 << 9), /* device supports DIPM */
+ ATA_DFLAG_DMADIR= (1 << 10), /* device requires DMADIR */
ATA_DFLAG_CFG_MASK = (1 << 12) - 1,
ATA_DFLAG_PIO = (1 << 12), /* device limited to PIO mode */
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's needed for PMP support?
Hello, Mark. Mark Lord wrote: > Tejun, I've added PMP to sata_mv, and am now trying to get it > to work with a Marvell PM attached. > > And the behaviour I see is very bizarre. > > After hard+soft resets, the PM signature is found, > and libata interrogates the PM registers. > > It successfully reads register 0, and then register 1. > But all subsequent registers read out (incorrectly) as zeros. Hmmm... > I've traced the taskfiles in/out, and it all looks proper > except for the actual data coming back from the PM. > > After some experimentation, I found that all of the PM registers > were readable, if I simple inserted a sata_pmp_read(link, 0, &junk) > in front of each issue of sata_pmp_read(link, reg, &r_val). Hmmm... > Then the PM is recognized and all, but fails port enumerations > probably due to either my hack or the same original bug (whatever that is?) > > I'm confused. That makes two of us. > The PM itself works fine here on sata_sil24 and AHCI(jmicron), Yeah, Marvell PMPs behave very nicely with sil24 and ahci. > so it's obviously the sata_mv driver or chipset that's being weird. > > Ever seen anything like this before? No. > I'm trying to use stock libata functions for all of this where possible, > so there's not really that much new/necessary code in sata_mv for this. > I do force the PMP value for outgoing-FIS's (non standard register for it), > but that's about all that's custom here. > > The driver uses ATA register mode emulation for all commands > other than READ/WRITE disk stuff, including for the PM register accesses. > > Ever seen anything like this before? No but I had my fair share of problems doing PMP support for both sil24 and ahci. Any chance you can get access to a bus analyzer? I had a very weird problem w/ ahci PMP support which I don't think I could fix if I didn't have access to a bus analyzer at the time. (I don't remember details now tho.) -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Regression: kernel 2.6.24{,.1} ahci problem, does not boot (resend)
Malte Schröder wrote: >> Does irqpoll kernel parameter help? > > No, problem stays. Can you capture full boot log w/ irqpoll specified? If you have root filesystem connected to ahci, you'll probably have to use serial or netconsole. Also, please post full boot log from 2.6.23.11. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: align shost->hostdata to cacheline
James Bottomley wrote: > On Fri, 2008-02-15 at 07:49 +0900, Tejun Heo wrote: >> James Bottomley wrote: >>> On Thu, 2008-02-14 at 18:48 +0900, Tejun Heo wrote: >>>> shost->hostdata can contain arbitrary data including DMA target >>>> buffers. Align it to cacheline. >>>> >>>> Signed-off-by: Tejun Heo <[EMAIL PROTECTED]> >>>> --- >>>> James, what do you think? >>> Hmm, it will blow out the host size ... although that's not such a huge >>> problem since there are relatively few of them in most running kernels. >>> What's the actual use case for this, though? The host structure is >>> allocated in ordinary memory ... we don't make sure it's DMAable, and >>> most HBAs that want to use memory for mailboxes need coherent memory >>> anyway. >> "As it can contain arbitrary structure, it should follow the largest >> meaningful alignment to allow the contained structure proper alignment." >> is the logic. I think it's generally RTTD for inline private data but >> feel free to disagree. > > Well, the way we usually do that is to have the host float the alignment > if necessary. The problem with relying on __cacheline_aligned for an > allocated structure is that it only works if the allocated structure > actually begins on a cacheline. Kmalloc (which is where we get the host > from) doesn't necessarily obey this if certain slab debugging flags are > present. Hmmm... Right. That means libata either needs to create a separate slab for the sector buf or align it after allocating by itself. Eeeek. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sata_sil24 not configuring drive right?
Johann-Christoph Jacob wrote: > my problem is not with the transfer mode but with the NCQ queue_depth. > My bootlog shows: > ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 1) > but i would like it to show: > ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 31/32) > or something similar. > I was not exact enough concerning the writeability of > /sys/block/sda/device/queue_depth > The file is not read only but writing to it fails: > echo 31 > /sys/block/sda/device/queue_depth > bash: echo: write error: Invalid argument Right. >>> ATA device, with non-removable media >>> Model Number: WDC WD1200BEVS-07LAT0 >>> Serial Number: WD-WXE906801963 >>> Firmware Revision: 01.06M01 >>> Standards: >>> Supported: 7 6 5 4 >>> Likely used: 7 >>> Configuration: >>> Logical max current >>> cylinders 16383 16383 >>> heads 16 16 >>> sectors/track 63 63 >>> -- >>> CHS current addressable sectors: 16514064 >>> LBAuser addressable sectors: 234441648 >>> LBA48 user addressable sectors: 234441648 >>> device size with M = 1024*1024: 114473 MBytes >>> device size with M = 1000*1000: 120034 MBytes (120 GB) >>> Capabilities: >>> LBA, IORDY(can be disabled) >>> Queue depth: 1 Hmm... Here it is. Interesting. The drive is reporting queue depth of 1. Interesting. This is the first time I see this. The drive is telling libata that it supports NCQ and the max queue depth is 1 and libata configures it accordingly. I'll ask wd about it. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sata_mv: remove iounmap in mv_platform_remove and use devm_iomap
Mark Lord wrote: > Saeed Bishara wrote: >> this will fix crash bug when doing rmmod to the driver, this is >> because the >> port_stop function get called later and it could access the device's >> registers. >> >> Signed-off-by: Saeed Bishara <[EMAIL PROTECTED]> Acked-by: Tejun Heo <[EMAIL PROTECTED]> -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ata: fix sparse warning in libata.h
Harvey Harrison wrote: > Avoids lots of these, also is more readable. > include/linux/libata.h:1210:13: warning: potentially expensive pointer > subtraction > > Change the subtraction to addition on the other side of the comparison. > > Thanks to Christer Weinigel for the suggestion. > > Signed-off-by: Harvey Harrison <[EMAIL PROTECTED]> Acked-by: Tejun Heo <[EMAIL PROTECTED]> -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Andrew Morton wrote: >> So, I guess it's NACK w/o suggested alternatives, right? > > I wouldn't nack without good reasons, and I have none here. I don't have > very strong opinions either way. I was just wondering whether I should just go with snprintf dancing in eh_link_report, which does make sense if not many need merging printk. > As a seat-of-the-pants thing, it does seem to be a lot of core code to > solve a fairly minor problem in (afaik) one remote place. But I haven't > looked - perhaps there are other places which could be improved if such > facilities were available. Okay, I see. I'll look around and see whether there are other places which can use it. I can think of a few in SCSI. Let's see. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Andrew Morton wrote: >>> printk is a special case, I think. It's the primary logging/debugging >>> method which can't fail and as it's mostly interpreted by human beings >>> (and developers in problematic cases), it has different maneuvering room >>> on errors - ie. it's far better to print messages w/o header or proper >>> log level than failing to print, which is quite different requirements >>> from other components. >> Andrew, any more comments or suggestions on how to proceed on this? > > Nope. > >> One >> way or the other, I think this is a problem worth solving. > > There are a lot of such problems ;) So, I guess it's NACK w/o suggested alternatives, right? -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h
Harvey Harrison wrote: >>> I know it's ugly, but I had it done anyways. The one real problem I have >>> with it is that if link and ap->pmp_link ever get changed to different types >>> the compiler will not even warn as we cast away to (char *). To make it >>> a bit more robust, a BUILD_BUG_ON checking the pointer types may be a >>> good idea. >> Sorry, but Nacked-by: Tejun Heo <[EMAIL PROTECTED]> >> > > Can't say I really blame you, other than this one error, drivers/ata > builds almost sparse-clean, and I had it done anyways. I wonder if > a helper similar in spirit would be any better. This doesn't have > any typechecking, but perhaps that could be dealt with too. > > Ran into a similar problem with mmzone.h, akpm has the patch, but > maybe a helper (kernel.h?) would be cleaner. Or maybe it's just > better to live with the sparse warnings > > /* > * return the offset of the ptr from the base, in bytes. > */ > #define PTR_OFF(base, ptr) \ > ((char *)ptr - (char *)base) > > if (PTR_OFF(ap->pmp_link, ++link) < ap->nr_pmp_links * sizeof(*link)) > return link; Can you please explain why it warns against pointer substraction? Is it because it can generate division? Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Tejun Heo wrote: > Hello, > > Andrew Morton wrote: >> On Thu, 14 Feb 2008 09:40:51 +0900 >> Tejun Heo <[EMAIL PROTECTED]> wrote: >> >>> Can you please take a look at ata_eh_link_report() in >>> drivers/ata/libata-eh.c? >> I did. Punishment? > > Heh.. :-) > >>> Currently, it has some problems. >> Yes, and the patches do clean that up. > > Yeap, it does. > >> ho hum. What tends to happen with this sort of thing is that fi we merge >> it, it ends up getting used more often than one expected... > > Hmmm... Okay. mprintk being printk, I'm not too sure how it can go over > usual expectations but well, yeah, that actually is my expectation. > >> If you stand back and squint at it, there are quite a few places where we >> do this sort of thing: allocate a buffer, squirt characters into it, >> reallocating and/or flushing as we proceed. All sysfs and procfs read-side >> code, for a start... > > printk is a special case, I think. It's the primary logging/debugging > method which can't fail and as it's mostly interpreted by human beings > (and developers in problematic cases), it has different maneuvering room > on errors - ie. it's far better to print messages w/o header or proper > log level than failing to print, which is quite different requirements > from other components. Andrew, any more comments or suggestions on how to proceed on this? One way or the other, I think this is a problem worth solving. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC-UGLYPATCH] ata: small optimization in linux/libata.h
Harvey Harrison wrote: > This patch may be too ugly to live, it suppresses a lot of > sparse warnings in the libata build and produces slightly > tighter code. (4 instructions vs 5 and a few bytes saved). > > include/linux/libata.h:1214:13: warning: potentially expensive pointer > subtraction > > Original: > if (++link - ap->pmp_link < ap->nr_pmp_links) > return link; > > 52b: 89 d8 mov%ebx,%eax > 52d: 2b 82 60 26 00 00 sub0x2660(%edx),%eax > 533: c1 f8 02sar$0x2,%eax > 536: 69 c0 dd 3d c8 44 imul $0x44c83ddd,%eax,%eax > 53c: 3b 82 5c 26 00 00 cmp0x265c(%edx),%eax > 542: 7d 04 jge548 > > Next: > if ((char*)++link - (char *)ap->pmp_link < ap->nr_pmp_links * > sizeof(*link)) > return link; > > 52b: 69 81 5c 26 00 00 d4imul $0x9d4,0x265c(%ecx),%eax > 532: 09 00 00 > 535: 89 da mov%ebx,%edx > 537: 2b 91 60 26 00 00 sub0x2660(%ecx),%edx > 53d: 39 c2 cmp%eax,%edx > 53f: 73 04 jae545 > > Signed-off-by: Harvey Harrison <[EMAIL PROTECTED]> > --- > I know it's ugly, but I had it done anyways. The one real problem I have > with it is that if link and ap->pmp_link ever get changed to different types > the compiler will not even warn as we cast away to (char *). To make it > a bit more robust, a BUILD_BUG_ON checking the pointer types may be a > good idea. Sorry, but Nacked-by: Tejun Heo <[EMAIL PROTECTED]> -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Regression: kernel 2.6.24{,.1} ahci problem, does not boot (resend)
Malte Schröder wrote: > Hello, > on one of my machines neither 2.6.24 nor 2.6.24.1 work. > The system is 64bit on Athlon X2 and ATI-Chipset (SB600). > > Extract from the kernel messages during boot: > > [ 66.943103] ahci :00:12.0: controller can't do 64bit DMA, forcing 32bit > [ 66.950374] ahci :00:12.0: controller can't do PMP, turning off CAP_PMP > [ 67.956470] ahci :00:12.0: AHCI 0001.0100 32 slots 4 ports 3 Gbps 0xf > impl SATA mode > [ 67.964996] ahci :00:12.0: flags: ncq sntf ilck pm led clo pio slum > part > [ 67.972820] scsi0 : ahci > [ 67.975699] scsi1 : ahci > [ 67.978445] scsi2 : ahci > [ 67.981178] scsi3 : ahci > [ 67.983949] ata1: SATA max UDMA/133 abar [EMAIL PROTECTED] port 0xfadffd00 > irq 509 > [ 67.991825] ata2: SATA max UDMA/133 abar [EMAIL PROTECTED] port 0xfadffd80 > irq 509 > [ 67.999729] ata3: SATA max UDMA/133 abar [EMAIL PROTECTED] port 0xfadffe00 > irq 509 > [ 68.007619] ata4: SATA max UDMA/133 abar [EMAIL PROTECTED] port 0xfadffe80 > irq 509 > [ 68.470669] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) > [ 98.431945] ata1.00: qc timeout (cmd 0xec) > [ 98.454907] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4) > [ 98.461296] ata1: failed to recover some devices, retrying in 5 secs > [ 103.916773] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) > [ 133.878045] ata1.00: qc timeout (cmd 0xec) > [ 133.882371] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4) > [ 133.888797] ata1: failed to recover some devices, retrying in 5 secs > [ 139.343901] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) > [ 169.305174] ata1.00: qc timeout (cmd 0xec) > [ 169.309534] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4) > [ 169.315926] ata1: failed to recover some devices, retrying in 5 secs > [ 174.771030] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300) > > The complete boot-log (captured via serial console), lspci output, > output of hdparm -I and the kernel config are attached. > > The PC has one IDE-drive and four SATA-drives (see hdparm.txt). > If I wait long enough, the IDE-drive get's recognized and I can > continue the boot process, but the SATA-drives are never recognized. > The system work's fine with kernel 2.6.23.11 (later kernels not tested). Does irqpoll kernel parameter help? -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMicron - hard resetting link
Gabor FUNK wrote: > To be honest, I didn't believe that doing anything with the PSU > would do something. > However, seemingly it did. > I have also updated the BIOS, but I guess this has not much > to do with it. I too am amazed at the number of PSU problems getting reported here. It seems most hardware problems turn out to be power related. > So a different brand PSU was additionally installed, and this > one got the motherboard and the 4 disk which were failing. > The "old" PSU got the second 4 hdds and the 2 other system > HDDs. > Test was started yesterday (Feb 13) about 16:30 CET including > array building up and file copies. About today (14) 20:22 the > problem appeared, but seemingly "moved" with the PSU to the > other 4 disks bunch (on nvidia controller) - more precisely, only > 2 of them (array is still operational). Hmmm.. > So it seems that there is definitely something with the "old" PSU. > > Also, I tried to mount the failed drives, without success. > > Thought I let you know. > Now I will try with the only one, "new" PSU to see what happens... Yeah, please keep us posted. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: align shost->hostdata to cacheline
James Bottomley wrote: > On Thu, 2008-02-14 at 18:48 +0900, Tejun Heo wrote: >> shost->hostdata can contain arbitrary data including DMA target >> buffers. Align it to cacheline. >> >> Signed-off-by: Tejun Heo <[EMAIL PROTECTED]> >> --- >> James, what do you think? > > Hmm, it will blow out the host size ... although that's not such a huge > problem since there are relatively few of them in most running kernels. > What's the actual use case for this, though? The host structure is > allocated in ordinary memory ... we don't make sure it's DMAable, and > most HBAs that want to use memory for mailboxes need coherent memory > anyway. "As it can contain arbitrary structure, it should follow the largest meaningful alignment to allow the contained structure proper alignment." is the logic. I think it's generally RTTD for inline private data but feel free to disagree. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi: align shost->hostdata to cacheline
shost->hostdata can contain arbitrary data including DMA target
buffers. Align it to cacheline.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
James, what do you think?
include/scsi/scsi_host.h |8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5c58d59..3876d24 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -663,12 +663,10 @@ struct Scsi_Host {
void *shost_data;
/*
-* We should ensure that this is aligned, both for better performance
-* and also because some compilers (m68k) don't automatically force
-* alignment to a long boundary.
+* Used for storage of host specific stuff. As it may contain
+* arbitrary data, align it to cacheline.
*/
- unsigned long hostdata[0] /* Used for storage of host specific stuff */
- __attribute__ ((aligned (sizeof(unsigned long;
+ unsigned long hostdata[0] cacheline_aligned;
};
#defineclass_to_shost(d) \
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libata: align ap->sector_buf to cacheline
ap->sector_buf is used as DMA target and misalignment can cause data
corruption on non-coherent architectures. This problem is spotted and
initial patch is submitted by Mark Mason.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: Mark Mason <[EMAIL PROTECTED]>
---
I thought about it more and marking up with ___cacheline_aligned seems
to be the right thing to do. For older ones where ata_port is
allocated as part of scsi_host, I think what should be done is to
align private area of scsi_host to cacheline. Will post another patch
for that.
Thanks.
include/linux/libata.h |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
diff --git a/include/linux/libata.h b/include/linux/libata.h
index bc5a8d0..7604763 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -671,7 +671,8 @@ struct ata_port {
acpi_handle acpi_handle;
struct ata_acpi_gtm __acpi_init_gtm; /* use ata_acpi_init_gtm() */
#endif
- u8 sector_buf[ATA_SECT_SIZE]; /* owned by EH */
+ /* owned by EH, must be cache line aligned as it's used as DMA target */
+ u8 sector_buf[ATA_SECT_SIZE] cacheline_aligned;
};
struct ata_port_operations {
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: libata/sata_sil24 cache alignment problem?
Alan Cox wrote:
>> I hadn't considered that approach due to the way the ata_port is allocated:
>>
>>> libata-core.c:
>>> host = scsi_host_alloc(ent->sht, sizeof(struct ata_port));
>>>
>>> hosts.c:
>>> struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int
>>> privsize)
>>> {
>>> shost = kzalloc(sizeof(struct Scsi_Host) + privsize, gfp_mask);
>>> }
>> The ata_port allocation is tacked onto the end of the Scsi_Host
>> allocation, so the start of ata_port will only be cache aligned if the
>> end of the Scsi_Host struct is, although that would be easy enough to
>> fix since it's currently aligned to an unsigned long boundary.
>
> You are right.
For recent kernels, ata_port is allocated separately so
cacheline_aligned should be enough.
>> I like that approach better, since it's clearer what the intent is,
But, yeah, allocating separately is probably safer.
>> and it's easier. Is there any other way that the ata_port struct
>> might be used that would invalidate this?
>
> I can't think of one. The object lifetime is right - the ata_port is
> created before the port buffer is used and destroyed after it is finished
> (obviously or embedding it in the struct wouldn't work either)
I'll prep a patch for the current kernel. Hmmm... This means that any
DMA buffer should be aligned to cacheline. Block layer DMA alignment
helpers should probably take this into consideration and we better add
warnings to dma map functions. This only affects DMA-to-memory, right?
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pata_scc.c: add thaw ops
Akira Iguchi wrote: > This patch adds default thaw ops and fixes the freeze/thaw inconsistency. > > Signed-off-by: Kou Ishizaki <[EMAIL PROTECTED]> > Signed-off-by: Akira Iguchi <[EMAIL PROTECTED]> Acked-by: Tejun Heo <[EMAIL PROTECTED]> -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Hello, Andrew Morton wrote: > On Thu, 14 Feb 2008 09:40:51 +0900 > Tejun Heo <[EMAIL PROTECTED]> wrote: > >> Can you please take a look at ata_eh_link_report() in >> drivers/ata/libata-eh.c? > > I did. Punishment? Heh.. :-) >> Currently, it has some problems. > > Yes, and the patches do clean that up. Yeap, it does. > ho hum. What tends to happen with this sort of thing is that fi we merge > it, it ends up getting used more often than one expected... Hmmm... Okay. mprintk being printk, I'm not too sure how it can go over usual expectations but well, yeah, that actually is my expectation. > If you stand back and squint at it, there are quite a few places where we > do this sort of thing: allocate a buffer, squirt characters into it, > reallocating and/or flushing as we proceed. All sysfs and procfs read-side > code, for a start... printk is a special case, I think. It's the primary logging/debugging method which can't fail and as it's mostly interpreted by human beings (and developers in problematic cases), it has different maneuvering room on errors - ie. it's far better to print messages w/o header or proper log level than failing to print, which is quite different requirements from other components. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Andrew Morton wrote: >> And mprintk the following. >> >> code: >> DEFINE_MPRINTK(mp, 2 * 80); >> >> mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); >> mprintk_push(&mp, "ATA %d", 7); >> mprintk_push(&mp, ", %u sectors\n", 1024); >> mprintk(&mp, "everything seems dandy\n"); >> >> output: >> <6>ata1.00: ATA 7, 1024 sectors >> <6>ata1.00 everything seems dandy > > And that looks like an awful lot of fuss. Is it really worth doing? In the above example, s/mprintk(/mprintk_flush(/ and s/mprintk_push(/mprintk(/ Can you please take a look at ata_eh_link_report() in drivers/ata/libata-eh.c? Currently, it has some problems. * All that it wants to do is printing some messages but it's awfully complex with temp bufs and super-long printk calls containing optional %s's. * status / error decode print outs are printed separately from cmd/res making it difficult to tell how they are grouped. Putting them together would require allocating another temp buf(s) and adding extra %s's to cmd/res printout. * For timeouts, result TF isn't available and thus res printout is misleading. res shouldn't be printed after timeouts. This would require allocating yest another temp buf and separating out res printing into separate snprintf. I was trying to do this and got fed up with all the tricks in the function. The only sane way out is to build messages piece-by-piece into a buffer and print it at once. The eh message is gigantic and I needed to allocate the buffer elsewhere than stack. ata_eh_link_report() fortunately has fixed place for that - ap->sector_buf - but let's assume there was no such buffer for the discussion. I'm still not too sure whether it's wise to use sector_buf for message building anyway. The only way is to malloc the buffer. Once the buffer is available, building message using snprintf is a bit cumbersome but is okay. The problem is that malloc can fail. To handle that case, we basically need to do if (buf) printed += snprintf(buf + printed, len - printed, ...); else printk(...); which is very cumbersome, so we need a wrapper around the above. As the wrapper needs to control when the message goes out, a flush function is necessary too. Combine those with overflow handling - mprintk. Similar problem exists for ata_dev_configure() in drivers/ata/libata-core.c although it's a bit better there. Please take a look at the fifth patch. It doesn't remove a lot of lines but it streamlines both functions significantly. For ata_dev_configure(), message reporting becomes what the function does secondarily while configuring the device, not something it's structured around. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH #upstream] libata: implement libata.force module parameter
Mark Lord wrote: > Tejun Heo wrote: >> This patch implements libata.force module parameter which can >> selectively override ATA port, link and device configurations >> including cable type, SATA PHY SPD limit, transfer mode and NCQ. > ... >> +libata.force=[LIBATA] Force configurations. The format is comma >> +separated list of "[ID:]VAL" where ID is >> +PORT[:DEVICE]. PORT and DEVICE are decimal numbers >> +matching port, link or device. Basically, it matches > .. > > Mmm.. not a NAK, but is there also a way to set/change these on the fly? What do you mean by 'on the fly'? While the system is running? If so, I think that should be done through other interfaces - pass through, sysfs, etc... > I ask because, on my 4-core test system here, libata enumerates > the ports differently depending upon whether I boot with a 32-bit > kernel or a 64-bit kernel. > > Major PITA, that, and it's just the kind of thing that spoils > fixed "PORT:DEVICE" module parameters, too. > > Now mind you, it's more likely the PCI layer that does the reverse > order thing, but the end result is that my drives/ports are numbered > differently depending upon which kernel I happen to boot with. Heck... That's ugly. libata.force is mainly conceived as debugging / installation helper, so using fixed PORT is good enough but maybe allowing bus_id as PORT is useful? Something like [00:1f.2]:00? -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sil3114 corruptions (was: Re: [PATCH 3/3] faster workaround)
Bernd Schubert wrote: > Hello Tejun, > > On Tuesday 23 October 2007 10:08:01 you wrote: >> Jeff Garzik wrote: >>> Alan Cox wrote: > 2) Once we identified, over time, the set of drives affected by this > 3112 quirk (aka drives that didn't fully comply to SATA spec), the > debugging of corruption cases largely shifted to the standard > routine: update the BIOS, replace the > cables/RAM/power/mainboard/slot/etc. to be certain of problem location. Except for the continued series of later SI + Nvidia chipset (mostly) pattern which seems unanswered but also being later chips I assume unrelated to this problem. >>> The SIL_FLAG_MOD15WRITE flag is set in sil_port_info[] is set according >>> to the best info we have from SiI, which indicates that 3114 and 3512 do >>> not have the same problem as the 3112. >> I don't think this data corruption problem w/ sil3114 is related to >> m15w. m15w workaround slows down things quite a bit and is likely to >> hide problems on PCI bus side. There are reports of data corruption >> with 3114 on nvidia (most common), via and now amd chipsets. There's >> one on intel too but IIRC wasn't too definite. >> >> According to a user, freebsd didn't have data corruption problem on the >> same hardware. I copied PCI FIFO setup code (ours is broken BTW) but it >> didn't fix the problem. >> >> I'll try to reproduce the problem locally and hunt it down. > > the problem just came up here again, as you and Jeff already guessed also > with > the the workaround patches. > I guess you didn't find time to look into it yet? Is there anything I can do? > What would you have done after reproducing it? Jeff, any progress on testing? -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[libata-dev #upstream-fixes] pata_legacy: don't call ata_host_detach() after initialization failure
ata_host_detach() detaches an attached port and shouldn't be called on a port which hasn't been attached yet. pata_legacy incorrectly calls ata_host_detach() on unattached port after initialization failure causing oops. Fix it. Signed-off-by: Tejun Heo <[EMAIL PROTECTED]> Cc: Alan Cox <[EMAIL PROTECTED]> Cc: Ingo Molnar <[EMAIL PROTECTED]> Cc: Arjan van de Ven <[EMAIL PROTECTED]> --- drivers/ata/pata_legacy.c |2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c index 333dc15..7383f19 100644 --- a/drivers/ata/pata_legacy.c +++ b/drivers/ata/pata_legacy.c @@ -1278,8 +1278,6 @@ static __init int legacy_init_one(struct legacy_probe *probe) } } fail: - if (host) - ata_host_detach(host); platform_device_unregister(pdev); return ret; } - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] libata: make libata use printk_header() and mprintk
Reimplement libata printk helpers using printk_header, implement
helpers to initialize mprintk and use mprintk during device
configuration and EH reporting.
This fixes various formatting related problems of libata messages such
as misaligned multiline messages, decoded register lines with leading
headers making them difficult to tell to which error they belong to,
awkward manual indents and complex message printing logics. More
importantly, by making message assembly flexible, this patch makes
future changes to device configuration and EH reporting easier.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/libata-core.c | 204 +++
drivers/ata/libata-eh.c | 182 ++
drivers/ata/libata-pmp.c|5 +-
drivers/ata/libata-scsi.c |6 +-
drivers/ata/sata_inic162x.c |2 +-
drivers/ata/sata_nv.c |4 +-
include/linux/libata.h | 35
7 files changed, 237 insertions(+), 201 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 004dae4..da670c0 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -129,6 +129,79 @@ MODULE_LICENSE("GPL");
MODULE_VERSION(DRV_VERSION);
+int ata_port_printk(struct ata_port *ap, const char *lv, const char *fmt, ...)
+{
+ va_list args;
+ char hbuf[16];
+ int ret;
+
+ snprintf(hbuf, sizeof(hbuf), "%sata%u: ", lv, ap->print_id);
+
+ va_start(args, fmt);
+ ret = vprintk_header(hbuf, fmt, args);
+ va_end(args);
+
+ return ret;
+}
+
+int ata_link_printk(struct ata_link *link, const char *lv, const char *fmt,
...)
+{
+ va_list args;
+ char hbuf[16];
+ int ret;
+
+ if (link->ap->nr_pmp_links)
+ snprintf(hbuf, sizeof(hbuf), "%sata%u.%02u: ",
+lv, link->ap->print_id, link->pmp);
+ else
+ snprintf(hbuf, sizeof(hbuf), "%sata%u: ",
+lv, link->ap->print_id);
+
+ va_start(args, fmt);
+ ret = vprintk_header(hbuf, fmt, args);
+ va_end(args);
+
+ return ret;
+}
+
+int ata_dev_printk(struct ata_device *dev, const char *lv, const char *fmt,
...)
+{
+ va_list args;
+ char hbuf[16];
+ int ret;
+
+ snprintf(hbuf, sizeof(hbuf), "%sata%u.%02u: ",
+lv, dev->link->ap->print_id, dev->link->pmp + dev->devno);
+
+ va_start(args, fmt);
+ ret = vprintk_header(hbuf, fmt, args);
+ va_end(args);
+
+ return ret;
+}
+
+void ata_port_mp_header(struct ata_port *ap, const char *lv, struct mprintk
*mp)
+{
+ mprintk_set_header(mp, "%sata%u: ", lv, ap->print_id);
+}
+
+void ata_link_mp_header(struct ata_link *link, const char *lv,
+ struct mprintk *mp)
+{
+ if (link->ap->nr_pmp_links)
+ mprintk_set_header(mp, "%sata%u.%02u: ",
+ lv, link->ap->print_id, link->pmp);
+ else
+ mprintk_set_header(mp, "%sata%u: ", lv, link->ap->print_id);
+}
+
+void ata_dev_mp_header(struct ata_device *dev, const char *lv,
+ struct mprintk *mp)
+{
+ mprintk_set_header(mp, "%sata%u.%02u: ", lv, dev->link->ap->print_id,
+ dev->link->pmp + dev->devno);
+}
+
/**
* ata_tf_to_fis - Convert ATA taskfile to SATA FIS structure
* @tf: Taskfile to convert
@@ -2006,18 +2079,16 @@ static inline u8 ata_dev_knobble(struct ata_device *dev)
return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
}
-static void ata_dev_config_ncq(struct ata_device *dev,
- char *desc, size_t desc_sz)
+static void ata_dev_config_ncq(struct ata_device *dev, struct mprintk *mp)
{
struct ata_port *ap = dev->link->ap;
int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
- if (!ata_id_has_ncq(dev->id)) {
- desc[0] = '\0';
+ if (!ata_id_has_ncq(dev->id))
return;
- }
+
if (dev->horkage & ATA_HORKAGE_NONCQ) {
- snprintf(desc, desc_sz, "NCQ (not used)");
+ mprintk(mp, ", NCQ (not used)");
return;
}
if (ap->flags & ATA_FLAG_NCQ) {
@@ -2026,9 +2097,9 @@ static void ata_dev_config_ncq(struct ata_device *dev,
}
if (hdepth >= ddepth)
- snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
+ mprintk(mp, ", NCQ (depth %d)", ddepth);
else
- snprintf(desc, desc_sz, "NCQ (depth %d/%d)", hdepth, ddepth);
+ mprintk(mp, ", NCQ (depth %d/%d)", hdepth, ddepth
[PATCH 4/5] printk: add Documentation/printk.txt
Add Documentation/printk.txt which explains printk, mprintk and their
friends.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
Documentation/00-INDEX |2 +
Documentation/printk.txt | 724 ++
2 files changed, 726 insertions(+), 0 deletions(-)
create mode 100644 Documentation/printk.txt
diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
index 8d55670..e4c2862 100644
--- a/Documentation/00-INDEX
+++ b/Documentation/00-INDEX
@@ -313,6 +313,8 @@ powerpc/
- directory with info on using Linux with the PowerPC.
preempt-locking.txt
- info on locking under a preemptive kernel.
+printk.txt
+ - info on printk and mprintk.
prio_tree.txt
- info on radix-priority-search-tree use for indexing vmas.
ramdisk.txt
diff --git a/Documentation/printk.txt b/Documentation/printk.txt
new file mode 100644
index 000..e510a81
--- /dev/null
+++ b/Documentation/printk.txt
@@ -0,0 +1,724 @@
+printk and mprintk
+==
+Tejun Heo <[EMAIL PROTECTED]>, January 18 2008
+
+This document describes printk and mprintk.
+
+TABLE OF CONTENTS
+
+[1] printk
+[1-1] What is printk and how does it operate?
+[1-2] Log level
+ [1-2-1] What is log level and how do I use it?
+ [1-2-2] How the log level is treated by kernel and logging tools.
+[1-3] Timestamps
+[1-4] Multiline messages and printk_header
+[1-5] printk's friends
+
+[2] mprintk - the merging printk
+[2-1] What is mprintk?
+[2-2] How do I use mprintk?
+ [2-2-1] Initialization
+ [2-2-2] Assembling and printing
+ [2-2-3] Error handling
+[2-3] Some guidelines
+[2-4] mprintk's friends
+
+[3] How libata uses printk_header and mprintk
+
+
+[1] printk
+
+[1-1] What is printk and how does it operate?
+
+int printk(const char *fmt, ...)
+
+printk is the equivalent of print for perl scripts, echo for shell
+scripts and much more closely printf(3) for userland C programs. It
+takes the same arguments as printf(3), formats the string the same way
+and spits the result out. printk is the kernel's primary way to emit
+human-readable messages.
+
+printf(3) prints formatted messages to the default destination -
+standard output. printk also has default destination or rather
+destinations. Messages printed out via printk are stored in a message
+buffer which can be retrieved either by syslog(2) system call or
+reading /proc/kmsg and then printed to one or more registered
+consoles.
+
+The ring buffer is the asynchronous way to access the messages.
+dmesg(1) dumps its content, klogd reads the messages and logs the
+result via syslog facility. The ring buffer is of fixed size which
+can be configured by the 'log_buf_len' kernel parameter. If a new
+message is printed and the ring buffer is full, it wraps around and
+writes over the old messages.
+
+On the other hand, the consoles behave mostly synchronously{1}. The
+most common console device is the virtual console (your graphics
+adapter and monitor). While the machine is booting, if you disable or
+escape from the pretty graphical boot screen, you'll see lots of
+kernel messages scrolling by before the userland starts to run. All
+those messages are from printk and your monitor is serving as a
+console device. Other examples of console devices include serial
+console{2} and netconsole{3}.
+
+The synchronous operation of console devices can be very expensive
+depending on which console devices are attached. For example,
+transferring 160 characters over 9600 baud rate serial line consumes
+around 18.75 milliseconds and the CPU won't be doing anything else
+other than pushing the bytes out.
+
+Accesses to the log buffer are synchronized and messages from separate
+printk invocations never get intermixed; however, there is no
+guarantee that two consecutive calls to printk will show up
+consecutively. printk calls from other threads can be printed
+inbetween.
+
+Thread A Thread B
+
+printk("hello "); printk("The answer is ");
+printk("world!\n");printk("42\n");
+
+The output can be
+
+hello The answer is world!
+42
+
+or any other combination - 6 of them, so you shouldn't construct
+messages piece-by-piece by calling printk on substrings. You should
+build the complete message first and print it by calling printk once.
+As constructing a message from pieces can be a non-trival task, there
+is a helper mechanism called mprintk. It will be explained later.
+
+
+[1-2] Log level
+
+[1-2-1] What is log level and how do I use it?
+
+Another difference from printf(3) is that printk performs a bit of
+post-processing on the printed messages. Each line printed via printk
+has a log level which is priority or urgency of the message. Eight
+log levels are defined and have the following meanings.
+
+KERN_EMERG :
[PATCH 2/5] printk: implement [v]printk_header()
Implement [v]printk_header() which takes @header argument and
automatically prints header in front of or indents multiline messages.
For example, if @header is "<7>ata1.00: " and the formatted message is
"<6>line0\nline1\n", the following gets written to the console.
<6>ata1.00: line0
<6>ata1.00 line1
As seen above if header ends with ':' followed by any number of
spaces, the colon is replaced with space from the second line. This
helps to distinguish where a multiline message begins when messages of
similar pattern repeats, for example...
ata8.00: cmd 60/80:20:e1:71:02/00:00:00:00:00/40 tag 4 ncq 65536 in
ata8.00 res 40/00:34:de:71:02/00:00:00:00:00/40 Emask 0x10 (ATA bus error)
ata8.00 status: { DRDY }
ata8.00: cmd 60/0e:28:d0:71:02/00:00:00:00:00/40 tag 5 ncq 7168 in
ata8.00 res 40/00:34:de:71:02/00:00:00:00:00/40 Emask 0x10 (ATA bus error)
ata8.00 status: { DRDY }
ata8.00: cmd 60/7f:38:61:72:02/00:00:00:00:00/40 tag 7 ncq 65024 in
ata8.00 res 40/00:34:de:71:02/00:00:00:00:00/40 Emask 0x10 (ATA bus error)
ata8.00 status: { DRDY }
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
include/linux/kernel.h | 12 ++
kernel/printk.c| 95 +---
2 files changed, 102 insertions(+), 5 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2df44e7..d885208 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -177,6 +177,10 @@ struct pid;
extern struct pid *session_of_pgrp(struct pid *pgrp);
#ifdef CONFIG_PRINTK
+asmlinkage int vprintk_header(const char *header, const char *fmt, va_list
args)
+ __attribute__ ((format (printf, 2, 0)));
+asmlinkage int printk_header(const char *header, const char * fmt, ...)
+ __attribute__ ((format (printf, 2, 3))) __cold;
asmlinkage int vprintk(const char *fmt, va_list args)
__attribute__ ((format (printf, 1, 0)));
asmlinkage int printk(const char * fmt, ...)
@@ -192,6 +196,14 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int
ratelimit_burst);
extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
unsigned int interval_msec);
#else
+static inline int vprintk_header(const char *header, const char *s, va_list
args)
+ __attribute__ ((format (printf, 2, 0)));
+static inline int vprintk_header(const char *header, const char *s, va_list
args)
+{ return 0; }
+static inline int printk_header(const char *header, const char *s, ...)
+ __attribute__ ((format (printf, 2, 3)));
+static inline int __cold printk_header(const char *header, const char *s, ...)
+{ return 0; }
static inline int vprintk(const char *s, va_list args)
__attribute__ ((format (printf, 1, 0)));
static inline int vprintk(const char *s, va_list args) { return 0; }
diff --git a/kernel/printk.c b/kernel/printk.c
index 2317ec8..2283a75 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -608,18 +608,75 @@ asmlinkage int printk(const char *fmt, ...)
int r;
va_start(args, fmt);
- r = vprintk(fmt, args);
+ r = vprintk_header(NULL, fmt, args);
va_end(args);
return r;
}
+EXPORT_SYMBOL(printk);
+
+asmlinkage int vprintk(const char *fmt, va_list args)
+{
+ return vprintk_header(NULL, fmt, args);
+}
+EXPORT_SYMBOL(vprintk);
+
+/**
+ * printk_header - print a kernel message with header
+ * @header: header string
+ * @fmt: format string
+ *
+ * This is printk() with a twist. @header points to string which will
+ * be used as message header. If @header is NULL, this function is
+ * identical to printk().
+ *
+ * @header may contain loglevel in front of it. Each new line
+ * including the first one in the message formatted from @fmt can also
+ * have loglevel. When both exist, the latter is used. In multiline
+ * messages, log level is inherited from the previous line if not
+ * explicitly specified.
+ *
+ * @header is printed when a new line starts. If @header ends with
+ * ':' followed by spaces, the colon is replaced with space from the
+ * second line.
+ *
+ * For example, if @header is "<7>ata1.00: " and the formatted message
+ * is "<6>line0\nline1\n<4>line2\nline3\n", the following gets written to
+ * the console.
+ *
+ * <6>ata1.00: line0
+ * <6>ata1.00 line1
+ * <4>ata1.00 line2
+ * <4>ata1.00 line3
+ */
+asmlinkage int printk_header(const char *header, const char *fmt, ...)
+{
+ va_list args;
+ int r;
+
+ va_start(args, fmt);
+ r = vprintk_header(header, fmt, args);
+ va_end(args);
+
+ return r;
+}
+EXPORT_SYMBOL(printk_header);
static int is_loglevel(const char *p)
{
return p[0] == '<' && p[1] >= '0' && p[1] <= '7' && p[2] == '>';
}
-asmlinkage int vprintk(const char *fmt, va_list args
[PATCH 3/5] printk: implement merging printk
There often are times printk messages need to be assembled piece by
piece and it's usually done using one of the following methods.
* Calling printk() on partial message segments. This used to be quite
common but has a problem - the message can break up if someone else
prints something in the middle.
* Prepping a temp buffer and build message piece-by-piece using
snprintf(). This is the most generic solution but corner case
handling can be tedious and on overflow messages can be lost and
such overflows would go unnoticed if overflow detection isn't
properly done.
* Collect all the partial data and perform printk() once with all the
parameters. This is often combined with the second. This usually
works but printing messages can become non-trivial programming
problem and can get quite tedious if the message format varies
depending on data.
None of the above is quite satisfactory. This patch implement merging
printk - mprintk to deal with this problem. It's basically a helper
to construct message piece-by-piece into the specified buffer. The
caller still has to care about buffer size and how the buffer is
allocated but mprintk makes it easier.
* DEFINE_MPRINTK() macro makes it easy to define a mprintk instance
with on-stack buffer. Malloc'd buffer can also be used.
* Message is never lost. On buffer overflow, all queued and to be
queued messages are printed followed by warning message and stack
dump. The warning message and stack dump are printed only once per
mprintk instance. The caller also doesn't have to handle buffer
malloc failure. If buffer is initialized to NULL, mprintk simply
bypasses the messages to printk().
* Has good support for multiline messages. A mprintk instance can
have header associated with it and the header can have log level.
Header log level is used if a partial message doesn't specify log
level explicitly. When log level is specified explicitly in a
partial message, the log level is applied to the partial message
only.
A simple example.
DEFINE_MPRINTK(mp, 2 * 80);
mprintk_set_header(&mp, KERN_DEBUG "ata%u.%2u: ", 1, 0);
mprintk_push(&mp, "ATA %d", 7);
mprintk_push(&mp, ", %u sectors\n", 1024);
mprintk(&mp, "everything seems dandy\n");
Which will be printed like the following as a single message.
<7>ata1.00: ATA 7, 1024 sectors
<7>ata1.00 everything seems dandy
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
include/linux/kernel.h | 73
kernel/printk.c| 300 +++-
2 files changed, 370 insertions(+), 3 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d885208..a9cbda8 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -177,6 +177,27 @@ struct pid;
extern struct pid *session_of_pgrp(struct pid *pgrp);
#ifdef CONFIG_PRINTK
+struct mprintk {
+ char*buf;
+ char*body;
+ char*cur;
+ char*end;
+ unsigned intflags;
+};
+
+#define MPRINTK_INITIALIZER(_buf, _size) \
+ { \
+ .buf= _buf, \
+ .body = _buf, \
+ .cur= _buf, \
+ .end= _buf + _size, \
+ .flags = 0 \
+ }
+
+#define DEFINE_MPRINTK(name, size) \
+ char __##name##_buf[size]; \
+ struct mprintk name = MPRINTK_INITIALIZER(__##name##_buf, size)
+
asmlinkage int vprintk_header(const char *header, const char *fmt, va_list
args)
__attribute__ ((format (printf, 2, 0)));
asmlinkage int printk_header(const char *header, const char * fmt, ...)
@@ -185,6 +206,26 @@ asmlinkage int vprintk(const char *fmt, va_list args)
__attribute__ ((format (printf, 1, 0)));
asmlinkage int printk(const char * fmt, ...)
__attribute__ ((format (printf, 1, 2))) __cold;
+
+static inline void mprintk_init(struct mprintk *mp, char *buf, size_t size)
+{
+ *mp = (struct mprintk)MPRINTK_INITIALIZER(buf, size);
+}
+extern void mprintk_init_alloc(struct mprintk *mp, gfp_t gfp);
+extern void mprintk_free(struct mprintk *mp);
+extern int vmprintk(struct mprintk *mp, const char *fmt, va_list args)
+ __attribute__ ((format (printf, 2, 0)));
+extern int vmprintk_set_header(struct mprintk *mp, const char *fmt, va_list
args)
+ __attribute__ ((format (printf, 2, 0)));
+extern int vmprintk_flush(struct mprintk *mp, const char *fmt, va_list args)
+ __attribute__ ((format (printf, 2, 0)));
+e
[PATCHSET] printk: implement printk_header() and merging printk, take #3
Hello, all. This is the third take of implement-printk_header-and-mprintk patchset. Changes from the last take[L] are... * Now header is printed on every line of a multiline message. If the header ends with ':' followed by spaces. The colon is replaced with space from the second line. * s/mprintk/mprintk_flush/ and s/mprintk_add/mprintk/ as suggested. * mprintk_init_alloc() and mprintk_free() added to ease malloc'd buffer handling. No need to specify buffer size. Sizing is left to mprintk. This patchset aims to make printing multiline messages and assembling message piece-by-piece easier. In a nutshell, printk_header() lets you do the following atomically (against other messages). code: printk(KERN_INFO "ata1.00: ", "line0\nline1\nline2\n"); output: <6>ata1.00: line0 <6>ata1.00 line1 <6>ata1.00 line2 And mprintk the following. code: DEFINE_MPRINTK(mp, 2 * 80); mprintk_set_header(&mp, KERN_INFO "ata%u.%2u: ", 1, 0); mprintk_push(&mp, "ATA %d", 7); mprintk_push(&mp, ", %u sectors\n", 1024); mprintk(&mp, "everything seems dandy\n"); output: <6>ata1.00: ATA 7, 1024 sectors <6>ata1.00 everything seems dandy For more info, please read Documentation/printk.txt which is created by the fourth patch. This patchset is on top of linux-2.6#master (96b5a46e2a72dc1829370c87053e0cd558d58bc0) + printk-fix-possible-printk-buffer-overrun [1] + printk-implement-printk_buf-overflow-warning [2] and contains the following patches. 0001-printk-keep-log-level-on-multiline-messages.patch 0002-printk-implement-v-printk_header.patch 0003-printk-implement-merging-printk.patch 0004-printk-add-Documentation-printk.txt.patch 0005-libata-make-libata-use-printk_header-and-mprintk.patch Documentation/00-INDEX |2 Documentation/printk.txt| 724 drivers/ata/libata-core.c | 204 +++- drivers/ata/libata-eh.c | 182 +-- drivers/ata/libata-pmp.c|5 drivers/ata/libata-scsi.c |6 drivers/ata/sata_inic162x.c |2 drivers/ata/sata_nv.c |4 include/linux/kernel.h | 85 + include/linux/libata.h | 35 +- kernel/printk.c | 435 -- 11 files changed, 1453 insertions(+), 231 deletions(-) If this gets acked, I think it's best to push it through libata-dev as libata is the first user. Thanks. -- tejun [L] http://thread.gmane.org/gmane.linux.ide/27317 [1] http://article.gmane.org/gmane.linux.kernel/639018 [2] http://article.gmane.org/gmane.linux.kernel/639029 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] printk: keep log level on multiline messages
When printing multiline messages, printk() resets log level to
default_message_loglevel after the first line. This changes log level
unexpectedly when printing multiline messages.
For example, libata error messages are printed like the following.
<3>ata8.00: cmd 60/01:00:e0:71:02/00:00:00:00:00/40 tag 0 ncq 512 in
<4> res 40/00:34:de:71:02/00:00:00:00:00/40 Emask 0x10 (ATA bus error)
This patch makes printk use the log level of the last line if the
current line doesn't specify log level explicitly.
While at it, separate out is_loglevel() test. This will be used by
later changes.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
kernel/printk.c | 50 +++---
1 files changed, 23 insertions(+), 27 deletions(-)
diff --git a/kernel/printk.c b/kernel/printk.c
index 419cd47..2317ec8 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -614,6 +614,11 @@ asmlinkage int printk(const char *fmt, ...)
return r;
}
+static int is_loglevel(const char *p)
+{
+ return p[0] == '<' && p[1] >= '0' && p[1] <= '7' && p[2] == '>';
+}
+
asmlinkage int vprintk(const char *fmt, va_list args)
{
/* cpu currently holding logbuf_lock */
@@ -625,10 +630,11 @@ asmlinkage int vprintk(const char *fmt, va_list args)
static int log_level_unknown = 1;
static char printk_buf[PRINTK_BUF_LEN + sizeof(overflow_tag) - 1];
+ int last_lv = default_message_loglevel;
unsigned long flags;
int printed_len = 0;
int this_cpu;
- char *p;
+ const char *p;
boot_delay_msec();
@@ -682,47 +688,37 @@ asmlinkage int vprintk(const char *fmt, va_list args)
for (p = printk_buf; *p; p++) {
if (log_level_unknown) {
/* log_level_unknown signals the start of a new line */
+ int lv = last_lv;
+
+ if (is_loglevel(p)) {
+ lv = p[1] - '0';
+ p += 3;
+ printed_len -= 3;
+ }
+
+ emit_log_char('<');
+ emit_log_char(lv + '0');
+ emit_log_char('>');
+ printed_len += 3;
+
if (printk_time) {
- int loglev_char;
char tbuf[50], *tp;
unsigned tlen;
unsigned long long t;
unsigned long nanosec_rem;
- /*
-* force the log level token to be
-* before the time output.
-*/
- if (p[0] == '<' && p[1] >='0' &&
- p[1] <= '7' && p[2] == '>') {
- loglev_char = p[1];
- p += 3;
- printed_len -= 3;
- } else {
- loglev_char = default_message_loglevel
- + '0';
- }
t = cpu_clock(printk_cpu);
nanosec_rem = do_div(t, 10);
- tlen = sprintf(tbuf,
- "<%c>[%5lu.%06lu] ",
- loglev_char,
+ tlen = sprintf(tbuf, "[%5lu.%06lu] ",
(unsigned long)t,
nanosec_rem/1000);
for (tp = tbuf; tp < tbuf + tlen; tp++)
emit_log_char(*tp);
printed_len += tlen;
- } else {
- if (p[0] != '<' || p[1] < '0' ||
- p[1] > '7' || p[2] != '>') {
- emit_log_char('<');
- emit_log_char(default_message_loglevel
- + '0');
- emit_log_char('>');
- printed_len += 3;
- }
}
+
+ last_lv = lv;
log_level_unknown = 0;
if (!*p)
break;
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH #upstream] libata: implement libata.force module parameter
This patch implements libata.force module parameter which can
selectively override ATA port, link and device configurations
including cable type, SATA PHY SPD limit, transfer mode and NCQ.
For example, you can say "use 1.5Gbps for all fan-out ports attached
to the second port but allow 3.0Gbps for the PMP device itself, oh,
the device attached to the third fan-out port chokes on NCQ and
shouldn't go over UDMA4" by the following.
libata.force=2:1.5g,2.15:3.0g,2.03:noncq,udma4
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
Okay, the build failure is dependent on compiler version. 4.1.2 fails
but 4.2.1 is okay. I was using 4.2.1 so I didn't know about it.
const is dropped from the offending structure and comment is added.
Documentation/kernel-parameters.txt | 35 +++
drivers/ata/libata-core.c | 380 +++-
drivers/ata/libata-eh.c |8
drivers/ata/libata.h|1
4 files changed, 420 insertions(+), 4 deletions(-)
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 8fd5aa4..e06ccf4 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -945,6 +945,41 @@ and is between 256 and 4096 characters. It is defined in
the file
when set.
Format:
+ libata.force= [LIBATA] Force configurations. The format is comma
+ separated list of "[ID:]VAL" where ID is
+ PORT[:DEVICE]. PORT and DEVICE are decimal numbers
+ matching port, link or device. Basically, it matches
+ the ATA ID string printed on console by libata. If
+ the whole ID part is omitted, the last PORT and DEVICE
+ values are used. If ID hasn't been specified yet, the
+ configuration applies to all ports, links and devices.
+
+ If only DEVICE is omitted, the parameter applies to
+ the port and all links and devices behind it. DEVICE
+ number of 0 either selects the first device or the
+ first fan-out link behind PMP device. It does not
+ select the host link. DEVICE number of 15 selects the
+ host link and device attached to it.
+
+ The VAL specifies the configuration to force. As long
+ as there's no ambiguity shortcut notation is allowed.
+ For example, both 1.5 and 1.5G would work for 1.5Gbps.
+ The following configurations can be forced.
+
+ * Cable type: 40c, 80c, short40c, unk, ign or sata.
+ Any ID with matching PORT is used.
+
+ * SATA link speed limit: 1.5Gbps or 3.0Gbps.
+
+ * Transfer mode: pio[0-7], mwdma[0-4] and udma[0-7].
+ udma[/][16,25,33,44,66,100,133] notation is also
+ allowed.
+
+ * [no]ncq: Turn on or off NCQ.
+
+ If there are multiple matching configurations changing
+ the same attribute, the last one is used.
+
load_ramdisk= [RAM] List of ramdisks to load from floppy
See Documentation/ramdisk.txt.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3011919..7d752c0 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -87,6 +87,28 @@ static struct workqueue_struct *ata_wq;
struct workqueue_struct *ata_aux_wq;
+struct ata_force_param {
+ const char *name;
+ unsigned intcbl;
+ int spd_limit;
+ unsigned long xfer_mask;
+ unsigned inthorkage_on;
+ unsigned inthorkage_off;
+};
+
+struct ata_force_ent {
+ int port;
+ int device;
+ struct ata_force_param param;
+};
+
+static struct ata_force_ent *ata_force_tbl;
+static int ata_force_tbl_size;
+
+static char ata_force_param_buf[PAGE_SIZE] __initdata;
+module_param_string(force, ata_force_param_buf, sizeof(ata_force_param_buf),
0444);
+MODULE_PARM_DESC(force, "Force ATA configurations including cable type, link
speed and transfer mode (see Documentation/kernel-parameters.txt for details)");
+
int atapi_enabled = 1;
module_param(atapi_enabled, int, 0444);
MODULE_PARM_DESC(atapi_enabled, "Enable discovery of ATAPI devices (0=off,
1=on)");
@@ -130,6 +152,179 @@ MODULE_VERSION(DRV_VERSION);
/**
+ * ata_force_cbl - force cable type according to libata.force
+ * @link: ATA link of interest
+ *
+ * Force cable type according to libata.force and whine about it.
+ * The last entry which has matching port numb
Re: [PATCHSET libata-dev#upstream] clean up scsi_host_templates and ata_port_operations, take #2
Akira Iguchi wrote: > Tejun Heo wrote: >> * pata_scc should now have NULL thaw after conversion. > > I'm sorry. This is a bug. > Please use default thaw (ata_bmdma_thaw) for pata_scc. That change should be a separate patch. This patchset shouldn't change any actual behavior but adding ata_bmdma_thaw does change the behavior. Can you please submit a separate patch to fix it? Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMicron - hard resetting link
Hello, Gabor FUNK wrote: >> What I said was that timeouts occurring due to transmission errors >> should be recoverable. It seems like IRQ delivery didn't work probably >> due to screaming IRQ. I need to see the messages before the first >> relevant error message. It's always a good idea to post full kernel log >> from boot till failure. Things which don't seem relevant are often >> relevant. > Naturally. Full kern.log with boot: > http://www.huweb.hu/maques/tmp/jmicron/kern.log > (no edits, there are really only those 2 lines between Feb 6 and Feb 9's > 1st exception) Hmmm... Indeed. This is the first time this mode of failure is reported. > Previously there was kernel 2.6.23.9 and I noticed the following in > syslog by then: > Feb 6 19:10:19 storage1 kernel: ata4: D2H reg with I during NCQ, this > message won't be printed again > Feb 6 19:10:20 storage1 kernel: ata1: D2H reg with I during NCQ, this > message won't be printed again > Feb 6 19:10:20 storage1 kernel: ata2: D2H reg with I during NCQ, this > message won't be printed again > Feb 6 19:10:21 storage1 kernel: ata3: D2H reg with I during NCQ, this > message won't be printed again > > I googled and saw that there was some fixes related to this (maybe it > was you), so that's why we hoped that 2.6.24 will fix this. Actually the > above error messages were gone, but... Yeap, those are gone. >> Till now, none of this kind of problem has been tracked down to MB or >> the controller while 90% of hardware problems turned out to be power >> related. > I'll put a brand new, probably different PSU in the case and put the MB > and the 4 disks of the problematic controller on it, and put the 2 system > and other 4 disks to this one (or even another one). Yeap, please keep me posted. > Meanwhile I'd welcome if you have any suggestion why controller reset > causing a "fatal error"... > BTW, the drives were accessible after the array broke (when I got there). What do you mean by 'drives were accessible'? /dev/sdX nodes were accessible? -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMicron - hard resetting link
Gabor FUNK wrote: >> It shouldn't kill the RAID. Hmmm... The log is truncated. Can you >> please post full kernel log spanning from boot to array death? > > RAID "dies" because controller dies, then it loses 4 disks out of 8... > Actually, the server last time was up and running for 2 months. > Then when it failed the 1st time, I did some tests and it went on for > 3 days, including building the raid and heavy test file copy. > The full log from the 1st relevant error message till the death of > the array is here: > http://www.huweb.hu/maques/tmp/jmicron/syslog What I said was that timeouts occurring due to transmission errors should be recoverable. It seems like IRQ delivery didn't work probably due to screaming IRQ. I need to see the messages before the first relevant error message. It's always a good idea to post full kernel log from boot till failure. Things which don't seem relevant are often relevant. >> Move half of the drives to the new PSU and see whether the problem goes >> away. > > This is a new server, with a Chieftec GPS650AB, 650W PSU in it. > Though AFAIK a harddisk consumes around 10W, and I will try to use > more than one PSU-s. I've recently tracked down IO problems a server product line from a major (really, one of the top three) vendor to malfunctioning PSU, so don't trust the labeling too much. > The main problem is that I can't immediately see if it helps or not. > Even if it will work without this problem for a week, I can't be sure it > still will in 2 months... > Because of this - and because I believe that this problem related to the HW > (motherboard, chipset) - I'd rather just throw away the MB and use an > other one with two extra 4 port SATA cards. Till now, none of this kind of problem has been tracked down to MB or the controller while 90% of hardware problems turned out to be power related. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
How to verify sht-ops-conversion patch doesn't change anything
The fifth and sixth patch, cleanup-sht and cleanup-ops, can be verified not to change the final sht and ops by printing out all entries and comparing the values before and after. 1. Get the following git branch. http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=cleanup-sht-ops-verify-before git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git cleanup-sht-ops-verify-before Build the kernel with the libata modules to verify and run the following command. # insmod libata.ko; for i in $(ls *.ko | sort | grep -v libata.ko); do insmod $i; done; dmesg -c | grep -E 'SHT|OPS' | sed -r 's/^\[[ \.0-9]*\] (.*)$/\1/' | sort > ~/out.before If you need some libata modules built-in for rootfs, it's okay too. It will give the same result. 2. Get the following git branch. http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=cleanup-sht-ops-verify-after git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git cleanup-sht-ops-verify-after # insmod libata.ko; for i in $(ls *.ko | sort | grep -v libata.ko); do insmod $i; done; dmesg -c | grep -E 'SHT|OPS' | sed -r 's/^\[[ \.0-9]*\] (.*)$/\1/' | sort > ~/out.after 3. Use your favorite diff program to compare out.before and out.after. Graphical ones which show in-line differences will work best. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMicron - hard resetting link
Gabor FUNK wrote: > Hi list, > > I seem to have a bug with JMicron controller in a Gigabyte > GA-N680SLI-DQ6 motherboard. > http://www.gigabyte.com.tw/Support/Motherboard/BIOS_Model.aspx?ProductID=2460 > > Kernel is 2.6.24. > 10 on-board SATA connectors, 2+4*JMicron 20360/20363 + 4*nVidia MCP55 > 2*200GB disks (System - SW RAID1) on the JMicron controller and > 8*500 (Data - SW RAID6) - 4 on the JMicron, 4 on the nVidia controller. > > Under heavy load the JMicron controller gets exceptions, then eventually > "hard resetting link". > All 4 disks/connector, one after another. This of course "kills" the RAID It shouldn't kill the RAID. Hmmm... The log is truncated. Can you please post full kernel log spanning from boot to array death? > I'm lost. > Anyone seen such thing? What could it be? Hardware (MB, chipset, BIOS), > kernel (driver) or what? > Any suggestion? Kernel version to try, dispose hardware or shoot myself > in the head? One of common causes for this kind of problem is bad power and PSUs which are rated for high wattage aren't always good enough. Prepare a power supply (popular cheap $15 one should do) such that it can be powered up by itself. http://modtown.co.uk/mt/article2.php?id=psumod Move half of the drives to the new PSU and see whether the problem goes away. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Current qc_defer implementation may lead to infinite recursion
Elias Oltmanns wrote: > Tejun Heo <[EMAIL PROTECTED]> wrote: >> Elias Oltmanns wrote: >>> This proves that piix_qc_defer() has declined the same command 100 >>> times in succession. However, this will only happen if the status of >>> all the commands enqueued for one port hasn't changed in the >>> meantime. This suggests to me that the threads scheduled for command >>> execution and completion aren't served for some reason. Any ideas? >> Blocked counts of 1 will cause busy looping because when blk_run_queue() >> returns because it's recursing too deep, it schedules unplug work right >> away, so it will easily loop 100 times. Max blocked counts should be >> adjusted to two (needs some testing before actually submitting the >> change). But that still shouldn't cause any lock up. What happens if >> you remove the 100 times limit? Does the machine hang on IO? > > Yes, it does. In fact, I had already verified that before sending the > previous email. Hmmm it's supposed not to lock up although it can cause busy wait. I'll test it tomorrow. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH #upstream] libata: implement libata.force module parameter
Jeff Garzik wrote: > Tejun Heo wrote: >> This patch implements libata.force module parameter which can >> selectively override ATA port, link and device configurations >> including cable type, SATA PHY SPD limit, transfer mode and NCQ. >> >> For example, you can say "use 1.5Gbps for all fan-out ports attached >> to the second port but allow 3.0Gbps for the PMP device itself, oh, >> the device attached to the third fan-out port chokes on NCQ and >> shouldn't go over UDMA4" by the following. >> >> libata.force=2:1.5g,2.15:3.0g,2.03:noncq,udma4 >> >> Signed-off-by: Tejun Heo <[EMAIL PROTECTED]> >> --- >> I guess it's about time we add something like this. More than >> anything else this should help debugging and can serve as a last >> resort to work around problems. >> >> Thanks. >> >> Documentation/kernel-parameters.txt | 35 +++ >> drivers/ata/libata-core.c | 375 >> +++- >> drivers/ata/libata-eh.c |8 >> drivers/ata/libata.h|1 4 files changed, 415 >> insertions(+), 4 deletions(-) > > ACK, but it breaks the build due to section type conflicts: > > drivers/ata/libata-core.c:108: error: ata_force_param_buf causes a > section type conflict > > Given that the data is marked __initdata and the code is marked __init, > I cannot see the problem. Jeff, this no longer causes build failure whether libata is configured built-in or as a module. I have no idea what's going on but there doesn't seem to be a proper solution on the horizon yet. I think we can go ahead and commit this one and convert it to __initdataconst when it becomes available. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Current qc_defer implementation may lead to infinite recursion
Elias Oltmanns wrote:
> Tejun Heo <[EMAIL PROTECTED]> wrote:
>> Elias Oltmanns wrote:
>>> +static int piix_qc_defer(struct ata_queued_cmd *qc)
>>> +{
>>> + static struct ata_port *ap = NULL;
>>> + struct ata_queued_cmd qcmd[ATA_MAX_QUEUE];
>> missing static?
>
> Oh well, I must have been too tired already yesterday. There are a few
> more things I got wrong last time. Please see the new patch at the end
> of this email.
>
> This time I applied the patch to 2.6.24.1 and performed a
>
> # cat large-file > /dev/null &
> # tail -f /var/log/kern.log
>
> and aborted once the output of dump_stack() had occurred. This proves
> that piix_qc_defer() has declined the same command 100 times in
> succession. However, this will only happen if the status of all the
> commands enqueued for one port hasn't changed in the meantime. This
> suggests to me that the threads scheduled for command execution and
> completion aren't served for some reason. Any ideas?
Blocked counts of 1 will cause busy looping because when blk_run_queue()
returns because it's recursing too deep, it schedules unplug work right
away, so it will easily loop 100 times. Max blocked counts should be
adjusted to two (needs some testing before actually submitting the
change). But that still shouldn't cause any lock up. What happens if
you remove the 100 times limit? Does the machine hang on IO?
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/10] libata: make ata_pci_init_one() not use ops->irq_handler and pi->sht
ata_pci_init_one() is the only function which uses ops->irq_handler
and pi->sht. Other initialization functions take the same information
as arguments. This causes confusion and duplicate unused entries in
structures.
Make ata_pci_init_one() take sht as an argument and use ata_interrupt
implicitly. All current users use ata_interrupt and if different irq
handler is necessary open coding ata_pci_init_one() using
ata_prepare_sff_host() and ata_activate_sff_host can be done under ten
lines including error handling and driver which requires custom
interrupt handler is likely to require custom initialization anyway.
As ata_pci_init_one() was the last user of ops->irq_handler, this
patch also kills the field.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/ata_generic.c |3 +--
drivers/ata/libata-core.c |1 -
drivers/ata/libata-sff.c|7 ---
drivers/ata/pata_acpi.c |3 +--
drivers/ata/pata_ali.c |9 +
drivers/ata/pata_amd.c | 12 +---
drivers/ata/pata_artop.c|6 +-
drivers/ata/pata_atiixp.c |3 +--
drivers/ata/pata_cmd640.c |3 +--
drivers/ata/pata_cmd64x.c |8 +---
drivers/ata/pata_cs5530.c |4 +---
drivers/ata/pata_cs5535.c |3 +--
drivers/ata/pata_cs5536.c |3 +--
drivers/ata/pata_cypress.c |3 +--
drivers/ata/pata_efar.c |3 +--
drivers/ata/pata_hpt366.c |3 +--
drivers/ata/pata_hpt37x.c |8 +---
drivers/ata/pata_hpt3x2n.c |3 +--
drivers/ata/pata_it8213.c |3 +--
drivers/ata/pata_it821x.c |4 +---
drivers/ata/pata_jmicron.c |3 +--
drivers/ata/pata_marvell.c |4 +---
drivers/ata/pata_netcell.c |3 +--
drivers/ata/pata_ns87410.c |3 +--
drivers/ata/pata_ns87415.c |4 +---
drivers/ata/pata_oldpiix.c |3 +--
drivers/ata/pata_opti.c |3 +--
drivers/ata/pata_optidma.c |4 +---
drivers/ata/pata_pdc202xx_old.c |5 +
drivers/ata/pata_radisys.c |3 +--
drivers/ata/pata_rz1000.c |3 +--
drivers/ata/pata_sc1200.c |3 +--
drivers/ata/pata_serverworks.c |6 +-
drivers/ata/pata_sil680.c |4 +---
drivers/ata/pata_sis.c | 10 +-
drivers/ata/pata_sl82c105.c |4 +---
drivers/ata/pata_triflex.c |3 +--
drivers/ata/pata_via.c |8 +---
include/linux/libata.h |4 ++--
39 files changed, 42 insertions(+), 130 deletions(-)
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index 0b5b515..a912ee0 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -120,7 +120,6 @@ static int ata_generic_init_one(struct pci_dev *dev, const
struct pci_device_id
{
u16 command;
static const struct ata_port_info info = {
- .sht = &generic_sht,
.flags = ATA_FLAG_SLAVE_POSS,
.pio_mask = 0x1f,
.mwdma_mask = 0x07,
@@ -153,7 +152,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const
struct pci_device_id
if (dev->vendor == PCI_VENDOR_ID_AL)
ata_pci_clear_simplex(dev);
- return ata_pci_init_one(dev, ppi);
+ return ata_pci_init_one(dev, ppi, &generic_sht);
}
static struct pci_device_id ata_generic[] = {
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4bb8e1e..bb42dd5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -109,7 +109,6 @@ const struct ata_port_operations ata_sff_port_ops = {
.irq_on = ata_irq_on,
.port_start = ata_sff_port_start,
- .irq_handler= ata_interrupt,
};
const struct ata_port_operations ata_bmdma_port_ops = {
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 34cdab0..24f6dc5 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -818,6 +818,7 @@ int ata_pci_activate_sff_host(struct ata_host *host,
* ata_pci_init_one - Initialize/register PCI IDE host controller
* @pdev: Controller to be initialized
* @ppi: array of port_info, must be enough for two ports
+ * @sht: scsi_host_template to use when registering the host
*
* This is a helper function which can be called from a driver's
* xxx_init_one() probe function if the hardware uses traditional
@@ -838,7 +839,8 @@ int ata_pci_activate_sff_host(struct ata_host *host,
* Zero on success, negative on errno-based value on error.
*/
int ata_pci_init_one(struct pci_dev *pdev,
-const struct ata_port_info * const * ppi)
+const struct ata_port_info * const * ppi,
+struct scsi_host_template *sht)
{
struct device *dev = &pdev->dev;
const struct ata_port_info *pi = NULL;
@@
[PATCH 09/10] libata: kill port_info->sht and ->irq_handler
libata core layer doesn't care about sht or ->irq_handler. Those are
only of interest to the LLD during initialization. This is confusing
and has caused several drivers to have duplicate unused initializers
for these fields.
Currently only sata_nv uses these fields. Make sata_nv use
->private_data, which is supposed to carry LLD-specific information,
instead and kill ->sht and ->irq_handler. nv_pi_priv structure is
defined and struct literals are used to initialize private_data.
Notational overhead is negligible.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/sata_nv.c | 29 +
include/linux/libata.h |2 --
2 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 7b7ba0e..5637b08 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -466,58 +466,61 @@ static struct ata_port_operations nv_swncq_ops = {
.port_start = nv_swncq_port_start,
};
+struct nv_pi_priv {
+ irq_handler_t irq_handler;
+ struct scsi_host_template *sht;
+};
+
+#define NV_PI_PRIV(_irq_handler, _sht) \
+ &(struct nv_pi_priv){ .irq_handler = _irq_handler, .sht = _sht }
+
static const struct ata_port_info nv_port_info[] = {
/* generic */
{
- .sht= &nv_sht,
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
.udma_mask = NV_UDMA_MASK,
.port_ops = &nv_generic_ops,
- .irq_handler= nv_generic_interrupt,
+ .private_data = NV_PI_PRIV(nv_generic_interrupt, &nv_sht),
},
/* nforce2/3 */
{
- .sht= &nv_sht,
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
.udma_mask = NV_UDMA_MASK,
.port_ops = &nv_nf2_ops,
- .irq_handler= nv_nf2_interrupt,
+ .private_data = NV_PI_PRIV(nv_nf2_interrupt, &nv_sht),
},
/* ck804 */
{
- .sht= &nv_sht,
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
.udma_mask = NV_UDMA_MASK,
.port_ops = &nv_ck804_ops,
- .irq_handler= nv_ck804_interrupt,
+ .private_data = NV_PI_PRIV(nv_ck804_interrupt, &nv_sht),
},
/* ADMA */
{
- .sht= &nv_adma_sht,
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
ATA_FLAG_MMIO | ATA_FLAG_NCQ,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
.udma_mask = NV_UDMA_MASK,
.port_ops = &nv_adma_ops,
- .irq_handler= nv_adma_interrupt,
+ .private_data = NV_PI_PRIV(nv_adma_interrupt, &nv_adma_sht),
},
/* SWNCQ */
{
- .sht= &nv_swncq_sht,
.flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
ATA_FLAG_NCQ,
.pio_mask = NV_PIO_MASK,
.mwdma_mask = NV_MWDMA_MASK,
.udma_mask = NV_UDMA_MASK,
.port_ops = &nv_swncq_ops,
- .irq_handler= nv_swncq_interrupt,
+ .private_data = NV_PI_PRIV(nv_swncq_interrupt, &nv_swncq_sht),
},
};
@@ -2316,6 +2319,7 @@ static int nv_init_one(struct pci_dev *pdev, const struct
pci_device_id *ent)
{
static int printed_version;
const struct ata_port_info *ppi[] = { NULL, NULL };
+ struct nv_pi_priv *ipriv;
struct ata_host *host;
struct nv_host_priv *hpriv;
int rc;
@@ -2352,6 +2356,7 @@ static int nv_init_one(struct pci_dev *pdev, const struct
pci_device_id *ent)
}
ppi[0] = &nv_port_info[type];
+ ipriv = ppi[0]->private_data;
rc = ata_pci_prepare_sff_host(pdev, ppi, &host);
if (rc)
return rc;
@@ -2390,8 +2395,8 @@ static int nv_init_one(struct pci_dev *pdev, const struct
pci_device_id *ent)
nv_swncq_host_init(host);
pci_set_master(pdev);
- return ata_host_activate(host, pdev->irq, ppi[0]->irq_handler,
-IRQF_SHARED, ppi[0]->sht);
+ return ata_host_activate(host, pdev->irq, ipriv->irq_handler,
+IRQF_SHARED, ipriv->sht);
}
#ifdef CONF
[PATCH 08/10] libata: stop overloading port_info->private_data
port_info->private_data is currently used for two purposes - to record
private data about the port_info or to specify host->private_data to
use when allocating ata_host.
This overloading is confusing and counter-intuitive in that
port_info->private_data becomes host->private_data instead of
port->private_data. In addition, port_info and host don't correspond
to each other 1-to-1. Currently, the first non-NULL
port_info->private_data is used.
This patch makes port_info->private_data just be what it is -
private_data for the port_info where LLD can jot down extra info.
libata no longer sets host->private_data to the first non-NULL
port_info->private_data, @host_priv argument is added to
ata_pci_init_one() instead. LLDs which use ata_pci_init_one() can use
this argument to pass in pointer to host private data. LLDs which
don't should use init-register model anyway and can initialize
host->private_data directly.
Adding @host_priv instead of using init-register model for LLDs which
use ata_pci_init_one() is suggested by Alan Cox.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: Alan Cox <[EMAIL PROTECTED]>
---
drivers/ata/ata_generic.c |2 +-
drivers/ata/libata-core.c |2 --
drivers/ata/libata-sff.c|4 +++-
drivers/ata/pata_acpi.c |2 +-
drivers/ata/pata_ali.c |2 +-
drivers/ata/pata_amd.c | 10 +-
drivers/ata/pata_artop.c|2 +-
drivers/ata/pata_atiixp.c |2 +-
drivers/ata/pata_cmd640.c |2 +-
drivers/ata/pata_cmd64x.c |2 +-
drivers/ata/pata_cs5530.c |2 +-
drivers/ata/pata_cs5535.c |2 +-
drivers/ata/pata_cs5536.c |2 +-
drivers/ata/pata_cypress.c |2 +-
drivers/ata/pata_efar.c |2 +-
drivers/ata/pata_hpt366.c | 12 ++--
drivers/ata/pata_hpt37x.c | 33 ++---
drivers/ata/pata_hpt3x2n.c |9 -
drivers/ata/pata_it8213.c |2 +-
drivers/ata/pata_it821x.c |2 +-
drivers/ata/pata_jmicron.c |2 +-
drivers/ata/pata_marvell.c |2 +-
drivers/ata/pata_netcell.c |2 +-
drivers/ata/pata_ns87410.c |2 +-
drivers/ata/pata_ns87415.c |2 +-
drivers/ata/pata_oldpiix.c |2 +-
drivers/ata/pata_opti.c |2 +-
drivers/ata/pata_optidma.c |2 +-
drivers/ata/pata_pdc202xx_old.c |2 +-
drivers/ata/pata_radisys.c |2 +-
drivers/ata/pata_rz1000.c |2 +-
drivers/ata/pata_sc1200.c |2 +-
drivers/ata/pata_serverworks.c |2 +-
drivers/ata/pata_sil680.c |2 +-
drivers/ata/pata_sis.c |8 +++-
drivers/ata/pata_sl82c105.c |2 +-
drivers/ata/pata_triflex.c |2 +-
drivers/ata/pata_via.c | 19 ---
include/linux/libata.h |2 +-
39 files changed, 74 insertions(+), 85 deletions(-)
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index a912ee0..b23e2a1 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -152,7 +152,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const
struct pci_device_id
if (dev->vendor == PCI_VENDOR_ID_AL)
ata_pci_clear_simplex(dev);
- return ata_pci_init_one(dev, ppi, &generic_sht);
+ return ata_pci_init_one(dev, ppi, &generic_sht, NULL);
}
static struct pci_device_id ata_generic[] = {
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bb42dd5..685e3c7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6905,8 +6905,6 @@ struct ata_host *ata_host_alloc_pinfo(struct device *dev,
if (!host->ops && (pi->port_ops != &ata_dummy_port_ops))
host->ops = pi->port_ops;
- if (!host->private_data && pi->private_data)
- host->private_data = pi->private_data;
}
return host;
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 24f6dc5..538b4af 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -819,6 +819,7 @@ int ata_pci_activate_sff_host(struct ata_host *host,
* @pdev: Controller to be initialized
* @ppi: array of port_info, must be enough for two ports
* @sht: scsi_host_template to use when registering the host
+ * @host_priv: host private_data
*
* This is a helper function which can be called from a driver's
* xxx_init_one() probe function if the hardware uses traditional
@@ -840,7 +841,7 @@ int ata_pci_activate_sff_host(struct ata_host *host,
*/
int ata_pci_init_one(struct pci_dev *pdev,
const struct ata_port_info * const * ppi,
-struct scsi_host_template *sht)
+struct scsi_host_template *sht, void *host_p
[PATCH 02/10] libata: reorganize ata_port_operations
Over the time, ops in ata_port_operations has become a bit confusing.
Reorganize. SFF/BMDMA ops are separated into separate a group as they
will be taken out of ata_port_operations later.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
include/linux/libata.h | 117 +---
1 files changed, 61 insertions(+), 56 deletions(-)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a7882a2..7e012d9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -672,69 +672,74 @@ struct ata_port {
};
struct ata_port_operations {
- void (*dev_config) (struct ata_device *);
+ /*
+* Command execution
+*/
+ int (*qc_defer)(struct ata_queued_cmd *qc);
+ int (*check_atapi_dma)(struct ata_queued_cmd *qc);
+ void (*qc_prep)(struct ata_queued_cmd *qc);
+ unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
- void (*set_piomode) (struct ata_port *, struct ata_device *);
- void (*set_dmamode) (struct ata_port *, struct ata_device *);
- unsigned long (*mode_filter) (struct ata_device *, unsigned long);
+ /*
+* Configuration and exception handling
+*/
+ int (*cable_detect)(struct ata_port *ap);
+ unsigned long (*mode_filter)(struct ata_device *dev, unsigned long
xfer_mask);
+ void (*set_piomode)(struct ata_port *ap, struct ata_device *dev);
+ void (*set_dmamode)(struct ata_port *ap, struct ata_device *dev);
+ int (*set_mode)(struct ata_link *link, struct ata_device
**r_failed_dev);
- void (*tf_load) (struct ata_port *ap, const struct ata_taskfile *tf);
- void (*tf_read) (struct ata_port *ap, struct ata_taskfile *tf);
+ void (*dev_config)(struct ata_device *dev);
- void (*exec_command)(struct ata_port *ap, const struct ata_taskfile
*tf);
+ void (*freeze)(struct ata_port *ap);
+ void (*thaw)(struct ata_port *ap);
+ void (*error_handler)(struct ata_port *ap);
+ void (*post_internal_cmd)(struct ata_queued_cmd *qc);
+
+ /*
+* Optional features
+*/
+ int (*scr_read)(struct ata_port *ap, unsigned int sc_reg, u32 *val);
+ int (*scr_write)(struct ata_port *ap, unsigned int sc_reg, u32 val);
+ void (*pmp_attach)(struct ata_port *ap);
+ void (*pmp_detach)(struct ata_port *ap);
+ int (*enable_pm)(struct ata_port *ap, enum link_pm policy);
+ void (*disable_pm)(struct ata_port *ap);
+
+ /*
+* Start, stop, suspend and resume
+*/
+ int (*port_suspend)(struct ata_port *ap, pm_message_t mesg);
+ int (*port_resume)(struct ata_port *ap);
+ int (*port_start)(struct ata_port *ap);
+ void (*port_stop)(struct ata_port *ap);
+ void (*host_stop)(struct ata_host *host);
+
+ /*
+* SFF / taskfile oriented ops
+*/
+ void (*dev_select)(struct ata_port *ap, unsigned int device);
u8 (*check_status)(struct ata_port *ap);
u8 (*check_altstatus)(struct ata_port *ap);
- void (*dev_select)(struct ata_port *ap, unsigned int device);
-
- void (*phy_reset) (struct ata_port *ap); /* obsolete */
- int (*set_mode) (struct ata_link *link, struct ata_device
**r_failed_dev);
-
- int (*cable_detect) (struct ata_port *ap);
-
- int (*check_atapi_dma) (struct ata_queued_cmd *qc);
-
- void (*bmdma_setup) (struct ata_queued_cmd *qc);
- void (*bmdma_start) (struct ata_queued_cmd *qc);
-
- unsigned int (*data_xfer) (struct ata_device *dev, unsigned char *buf,
- unsigned int buflen, int rw);
-
- int (*qc_defer) (struct ata_queued_cmd *qc);
- void (*qc_prep) (struct ata_queued_cmd *qc);
- unsigned int (*qc_issue) (struct ata_queued_cmd *qc);
-
- /* port multiplier */
- void (*pmp_attach) (struct ata_port *ap);
- void (*pmp_detach) (struct ata_port *ap);
-
- /* Error handlers. ->error_handler overrides ->eng_timeout and
-* indicates that new-style EH is in place.
+ void (*tf_load)(struct ata_port *ap, const struct ata_taskfile *tf);
+ void (*tf_read)(struct ata_port *ap, struct ata_taskfile *tf);
+ void (*exec_command)(struct ata_port *ap, const struct ata_taskfile
*tf);
+ unsigned int (*data_xfer)(struct ata_device *dev, unsigned char *buf,
+ unsigned int buflen, int rw);
+ u8 (*irq_on)(struct ata_port *);
+
+ void (*irq_clear)(struct ata_port *);
+ void (*bmdma_setup)(struct ata_queued_cmd *qc);
+ void (*bmdma_start)(struct ata_queued_cmd *qc);
+ void (*bmdma_stop)(struct ata_queued_cmd *qc);
+ u8 (*bmdma_status)(struct ata_port *ap);
+
+ /*
+* Obsolete
*/
- void (*eng_timeout) (struct ata_port *ap); /* obsolete */
-
- void (*freeze) (struct ata_port *ap);
- void (*thaw) (struct
[PATCH 03/10] libata: implement and use ata_noop_irq_clear()
->irq_clear() is used to clear IRQ bit of a SFF controller and isn't
useful for drivers which don't use libata SFF HSM implementation.
However, it's a required callback and many drivers implement their own
noop version as placeholder. This patch implements ata_noop_irq_clear
and use it to replace those custom placeholders.
Also, SFF drivers which don't support BMDMA don't need to use
ata_bmdma_irq_clear(). It becomes noop if BMDMA address isn't
initialized. Convert them to use ata_noop_irq_clear().
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/ahci.c | 12 +++-
drivers/ata/libata-core.c|1 +
drivers/ata/libata-sff.c |8
drivers/ata/pata_ali.c |2 +-
drivers/ata/pata_at32.c |7 +--
drivers/ata/pata_icside.c|7 +--
drivers/ata/pata_isapnp.c|2 +-
drivers/ata/pata_ixp4xx_cf.c |2 +-
drivers/ata/pata_legacy.c| 22 +++---
drivers/ata/pata_mpc52xx.c |2 +-
drivers/ata/pata_mpiix.c |2 +-
drivers/ata/pata_ns87410.c |2 +-
drivers/ata/pata_pcmcia.c|4 ++--
drivers/ata/pata_platform.c |2 +-
drivers/ata/pata_qdi.c |4 ++--
drivers/ata/pata_winbond.c |2 +-
drivers/ata/pdc_adma.c |8 +---
drivers/ata/sata_fsl.c |7 +--
drivers/ata/sata_inic162x.c |7 +--
drivers/ata/sata_mv.c| 11 +++
drivers/ata/sata_qstor.c |8 +---
drivers/ata/sata_sil24.c |8 +---
include/linux/libata.h |1 +
23 files changed, 46 insertions(+), 85 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 44946bd..415cc77 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -238,7 +238,6 @@ static int ahci_scr_read(struct ata_port *ap, unsigned int
sc_reg, u32 *val);
static int ahci_scr_write(struct ata_port *ap, unsigned int sc_reg, u32 val);
static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
*ent);
static unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
-static void ahci_irq_clear(struct ata_port *ap);
static int ahci_port_start(struct ata_port *ap);
static void ahci_port_stop(struct ata_port *ap);
static void ahci_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
@@ -298,7 +297,7 @@ static const struct ata_port_operations ahci_ops = {
.qc_prep= ahci_qc_prep,
.qc_issue = ahci_qc_issue,
- .irq_clear = ahci_irq_clear,
+ .irq_clear = ata_noop_irq_clear,
.scr_read = ahci_scr_read,
.scr_write = ahci_scr_write,
@@ -334,7 +333,7 @@ static const struct ata_port_operations ahci_vt8251_ops = {
.qc_prep= ahci_qc_prep,
.qc_issue = ahci_qc_issue,
- .irq_clear = ahci_irq_clear,
+ .irq_clear = ata_noop_irq_clear,
.scr_read = ahci_scr_read,
.scr_write = ahci_scr_write,
@@ -368,7 +367,7 @@ static const struct ata_port_operations ahci_p5wdh_ops = {
.qc_prep= ahci_qc_prep,
.qc_issue = ahci_qc_issue,
- .irq_clear = ahci_irq_clear,
+ .irq_clear = ata_noop_irq_clear,
.scr_read = ahci_scr_read,
.scr_write = ahci_scr_write,
@@ -1710,11 +1709,6 @@ static void ahci_port_intr(struct ata_port *ap)
}
}
-static void ahci_irq_clear(struct ata_port *ap)
-{
- /* TODO */
-}
-
static irqreturn_t ahci_interrupt(int irq, void *dev_instance)
{
struct ata_host *host = dev_instance;
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 410f1ea..2772657 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -7556,6 +7556,7 @@ EXPORT_SYMBOL_GPL(ata_noop_qc_prep);
EXPORT_SYMBOL_GPL(ata_bmdma_setup);
EXPORT_SYMBOL_GPL(ata_bmdma_start);
EXPORT_SYMBOL_GPL(ata_bmdma_irq_clear);
+EXPORT_SYMBOL_GPL(ata_noop_irq_clear);
EXPORT_SYMBOL_GPL(ata_bmdma_status);
EXPORT_SYMBOL_GPL(ata_bmdma_stop);
EXPORT_SYMBOL_GPL(ata_bmdma_freeze);
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 60cd4b1..34cdab0 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -297,6 +297,14 @@ void ata_bmdma_irq_clear(struct ata_port *ap)
}
/**
+ * ata_noop_irq_clear - Noop placeholder for irq_clear
+ * @ap: Port associated with this ATA transaction.
+ */
+void ata_noop_irq_clear(struct ata_port *ap)
+{
+}
+
+/**
* ata_bmdma_status - Read PCI IDE BMDMA status
* @ap: Port associated with this ATA transaction.
*
diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 5ef6594..8a9044c 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -342,7 +342,7 @@ static struct ata_port_operations ali_early_port_ops = {
[PATCH 04/10] libata: normalize port_info, port_operations and sht tables
Over the time, port info, ops and sht structures developed quite a bit
of inconsistencies. This patch updates drivers.
* Enable/disable_pm callbacks added to all ahci ops tables.
* Every driver for SFF controllers now uses ata_sff_port_start()
instead of ata_port_start() unless the driver has custom
implementation.
* Every driver for SFF controllers now uses ata_pci_default_filter()
unless the driver has custom implementation.
* Removed an odd port_info->sht initialization from ata_piix.c.
Likely a merge byproduct.
* A port which has ATA_FLAG_SATA set doesn't need to set cable_detect
to ata_cable_sata(). Remove it from via and mv port ops.
* Some drivers had unnecessary .max_sectors initialization which is
ignored and was missing .slave_destroy callback. Fixed.
* Removed unnecessary sht initializations port_info's.
* Removed onsolete scsi device suspend/resume callbacks from
pata_bf54x.
* No reason to set ata_pci_default_filter() and bmdma functions for
PIO-only drivers. Remove those callbacks and replace
ata_bmdma_irq_clear with ata_noop_irq_clear.
* pata_platform sets port_start to ata_dummy_ret0. port_start can
just be set to NULL.
* sata_fsl supports NCQ but was missing qc_defer. Fixed.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/ahci.c |4
drivers/ata/ata_generic.c|1 +
drivers/ata/ata_piix.c | 13 +++--
drivers/ata/pata_artop.c |1 +
drivers/ata/pata_bf54x.c |1 -
drivers/ata/pata_cmd64x.c|6 +++---
drivers/ata/pata_cs5520.c|1 +
drivers/ata/pata_cs5536.c|2 +-
drivers/ata/pata_hpt3x3.c|1 -
drivers/ata/pata_it8213.c|2 +-
drivers/ata/pata_ixp4xx_cf.c |3 +--
drivers/ata/pata_jmicron.c |3 ++-
drivers/ata/pata_marvell.c |1 +
drivers/ata/pata_mpc52xx.c |4 ++--
drivers/ata/pata_netcell.c |1 +
drivers/ata/pata_opti.c |7 +--
drivers/ata/pata_optidma.c |2 ++
drivers/ata/pata_platform.c |4
drivers/ata/pata_rz1000.c|7 +--
drivers/ata/sata_fsl.c |1 +
drivers/ata/sata_mv.c|6 --
drivers/ata/sata_nv.c| 11 ---
drivers/ata/sata_sil.c |3 ++-
drivers/ata/sata_sis.c |3 ++-
drivers/ata/sata_svw.c |3 ++-
drivers/ata/sata_uli.c |3 ++-
drivers/ata/sata_via.c | 12
drivers/ata/sata_vsc.c |3 ++-
28 files changed, 57 insertions(+), 52 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 415cc77..8ff885a 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -351,6 +351,8 @@ static const struct ata_port_operations ahci_vt8251_ops = {
.port_suspend = ahci_port_suspend,
.port_resume= ahci_port_resume,
#endif
+ .enable_pm = ahci_enable_alpm,
+ .disable_pm = ahci_disable_alpm,
.port_start = ahci_port_start,
.port_stop = ahci_port_stop,
@@ -385,6 +387,8 @@ static const struct ata_port_operations ahci_p5wdh_ops = {
.port_suspend = ahci_port_suspend,
.port_resume= ahci_port_resume,
#endif
+ .enable_pm = ahci_enable_alpm,
+ .disable_pm = ahci_disable_alpm,
.port_start = ahci_port_start,
.port_stop = ahci_port_stop,
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index 2053420..db4c3cb 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -114,6 +114,7 @@ static struct scsi_host_template generic_sht = {
static struct ata_port_operations generic_port_ops = {
.set_mode = generic_set_mode,
+ .mode_filter= ata_pci_default_filter,
.tf_load= ata_tf_load,
.tf_read= ata_tf_read,
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 9c2515f..6a32839 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -336,7 +336,7 @@ static const struct ata_port_operations piix_pata_ops = {
.irq_clear = ata_bmdma_irq_clear,
.irq_on = ata_irq_on,
- .port_start = ata_port_start,
+ .port_start = ata_sff_port_start,
};
static const struct ata_port_operations ich_pata_ops = {
@@ -367,7 +367,7 @@ static const struct ata_port_operations ich_pata_ops = {
.irq_clear = ata_bmdma_irq_clear,
.irq_on = ata_irq_on,
- .port_start = ata_port_start,
+ .port_start = ata_sff_port_start,
};
static const struct ata_port_operations piix_sata_ops = {
@@ -385,6 +385,7 @@ static const struct ata_port_operations piix_sata_ops = {
.qc_issue = ata_qc_issue_prot,
.data_xfer = ata_data_xfer,
+
[PATCH 01/10] libata: PCI device should be powered up before being accessed
PCI device should be powered up or powered up before its PCI regsiters
are accessed. Although PCI configuration register access is allowed
in D3hot, PCI device is free to reset its status when transiting from
D3hot to D0 causing configuration data to change.
Many libata SFF drivers which use ata_pci_init_one() read and update
configuration registers before calling ata_pci_init_one() which
enables the PCI device. Also, in resume paths, some drivers access
registers without resuming the PCI device.
This patch adds a call to pcim_enable_device() in init path if
register is accessed before calling ata_pci_init_one() and make resume
paths first resume PCI devices, access PCI configuration regiters then
resume ATA host.
While at it...
* cmd640 was strange in that it set ->resume even when CONFIG_PM is
not. This is by-product of minimal build fix. Updated.
* In cs5530, Don't BUG() on reinit failure. Just whine and fail
resume.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/pata_ali.c | 14 +-
drivers/ata/pata_amd.c | 16 +++-
drivers/ata/pata_artop.c |5 +
drivers/ata/pata_cmd640.c | 21 -
drivers/ata/pata_cmd64x.c | 15 ++-
drivers/ata/pata_cs5520.c | 15 ++-
drivers/ata/pata_cs5530.c | 18 --
drivers/ata/pata_hpt366.c | 14 +-
drivers/ata/pata_hpt37x.c |5 +
drivers/ata/pata_hpt3x2n.c |5 +
drivers/ata/pata_it821x.c | 14 +-
drivers/ata/pata_netcell.c |5 +
drivers/ata/pata_ns87415.c |6 ++
drivers/ata/pata_optidma.c |5 +
drivers/ata/pata_serverworks.c | 19 ---
drivers/ata/pata_sil680.c | 13 +++--
drivers/ata/pata_sis.c |6 +-
drivers/ata/pata_sl82c105.c|5 +
drivers/ata/pata_via.c | 14 +-
19 files changed, 195 insertions(+), 20 deletions(-)
diff --git a/drivers/ata/pata_ali.c b/drivers/ata/pata_ali.c
index 7e68edf..5ef6594 100644
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -597,6 +597,11 @@ static int ali_init_one(struct pci_dev *pdev, const struct
pci_device_id *id)
const struct ata_port_info *ppi[] = { NULL, NULL };
u8 tmp;
struct pci_dev *isa_bridge;
+ int rc;
+
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
/*
* The chipset revision selects the driver operations and
@@ -632,8 +637,15 @@ static int ali_init_one(struct pci_dev *pdev, const struct
pci_device_id *id)
#ifdef CONFIG_PM
static int ali_reinit_one(struct pci_dev *pdev)
{
+ struct ata_host *host = dev_get_drvdata(&pdev->dev);
+ int rc;
+
+ rc = ata_pci_device_do_resume(pdev);
+ if (rc)
+ return rc;
ali_init_chipset(pdev);
- return ata_pci_device_resume(pdev);
+ ata_host_resume(host);
+ return 0;
}
#endif
diff --git a/drivers/ata/pata_amd.c b/drivers/ata/pata_amd.c
index 761a666..567fe6b 100644
--- a/drivers/ata/pata_amd.c
+++ b/drivers/ata/pata_amd.c
@@ -662,10 +662,15 @@ static int amd_init_one(struct pci_dev *pdev, const
struct pci_device_id *id)
static int printed_version;
int type = id->driver_data;
u8 fifo;
+ int rc;
if (!printed_version++)
dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
+ rc = pcim_enable_device(pdev);
+ if (rc)
+ return rc;
+
pci_read_config_byte(pdev, 0x41, &fifo);
/* Check for AMD7409 without swdma errata and if found adjust type */
@@ -709,6 +714,13 @@ static int amd_init_one(struct pci_dev *pdev, const struct
pci_device_id *id)
#ifdef CONFIG_PM
static int amd_reinit_one(struct pci_dev *pdev)
{
+ struct ata_host *host = dev_get_drvdata(&pdev->dev);
+ int rc;
+
+ rc = ata_pci_device_do_resume(pdev);
+ if (rc)
+ return rc;
+
if (pdev->vendor == PCI_VENDOR_ID_AMD) {
u8 fifo;
pci_read_config_byte(pdev, 0x41, &fifo);
@@ -721,7 +733,9 @@ static int amd_reinit_one(struct pci_dev *pdev)
pdev->device == PCI_DEVICE_ID_AMD_COBRA_7401)
ata_pci_clear_simplex(pdev);
}
- return ata_pci_device_resume(pdev);
+
+ ata_host_resume(host);
+ return 0;
}
#endif
diff --git a/drivers/ata/pata_artop.c b/drivers/ata/pata_artop.c
index d421831..2f81480 100644
--- a/drivers/ata/pata_artop.c
+++ b/drivers/ata/pata_artop.c
@@ -446,11 +446,16 @@ static int artop_init_one (struct pci_dev *pdev, const
struct pci_device_id *id)
.port_ops = &artop6260_ops,
};
const struct ata_port_info *ppi[] = { NULL, NULL };
+ int rc;
[PATCHSET libata-dev#upstream] clean up scsi_host_templates and ata_port_operations, take #2
Hello, all. This is the second take of cleanup-sht-ops patchset. As the name suggests it reorganizes and cleans up scsi_host_template and ata_port_operation tables used by libata core and low level drivers. Please read the head message of the last take[L] for more info. Changes from the last take[L] are... * Updated to fit the current upstream. sata_mv should be fine now. * sht and ops conversions separated into two patches as Jeff requested. * Instead of using init-register model for SFF drivers which need to initialize host->private_data, @host_priv is added to ata_pci_init_one() as Alan requested. * pata_scc should now have NULL thaw after conversion. The following drivers need specific platform to build, so they need verification. If you work on one of the following drivers, please verify that the driver builds and works fine. It would be best if you can verify that the sht and ops don't change by the fifth and sixth patches using the method I'll write in another message. * pata_at32 * pata_bf54x * pata_icside * pata_ixp4xx_cf * pata_mpc52xx * pata_scc * sata_fsl This patchset is on top of upstream (bc5468f52b785ffa1fe0ea289baec2c51384d436) + prefer-hardreset patchset [1] + grace-failure-on-no-reset patch [2] + pci-allow-multiple-pcim_enable_device [3] This patchset is available as GIT branch. http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=cleanup-sht-ops git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git cleanup-sht-ops -- tejun [L] http://thread.gmane.org/gmane.linux.ide/27785 [1] http://thread.gmane.org/gmane.linux.ide/27447 [2] http://article.gmane.org/gmane.linux.ide/27781 [3] http://article.gmane.org/gmane.linux.ide/27782 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 2.6.24 found 5 exceptions in dmesg after not using the system for while
Andrew Paprocki wrote:
> I haven't used the system with these errors in a day or two and I came
> back and noticed 5 exceptions in dmesg. These are all the same:
>
> ata1: exception Emask 0x10 SAct 0x0 SErr 0x90200 action 0xe frozen
> ata1: irq_stat 0x0040, PHY RDY changed
> ata1: SError: { Persist PHYRdyChg 10B8B }
That's link going down and up again. Reseat/replace cable and see
whether the problem goes away. Connecting the drive to different port
is also worth trying. If none helps, the most likely cause is power.
After boot, record the result of 'smartctl -a' on the drive. After the
above conditions occur, do the same and compare the result. If
Start_Stop_Count increased, your power is having problem keeping the
drive spun up and if writes were in progress during such events, you'll
lose data.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sata_mv: problems using it as a platform_driver
Saeed Bishara wrote: >>> host->private_data = hpriv; >>> hpriv->n_ports = n_ports; >>> >>> -host->iomap = NULL; >>> hpriv->base = ioremap(res->start, res->end - res->start + 1); >>> +host->iomap = &hpriv->base; >>> hpriv->base -= MV_SATAHC0_REG_BASE; >>> >>> rc = mv_create_dma_pools(hpriv, &pdev->dev); >> .. >> >> Well, that's definitely one way to attack it. > the fix better be done by using the hpriv->base instead of iomap table: > void __iomem *hc_mmio = mv_hc_base_from_port( > - ap->host->iomap[MV_PRIMARY_BAR], hard_port); > + mv_host_base(ap->host), hard_port); > u32 hc_irq_cause, ipending; Yeah, that's the right thing to do. >> The original problem being, for a non-PCI device, there is no iomap[] >> table. >> sata_mv only ever uses iomap[MV_PRIMARY_BAR=0], so the above patch should >> work around it just fine. > Jeff, do the libata upper drivers use the other BARs? if not, then we can use > pci_iomap to map BAR0, this way we can remove the iomap[] table and use one > iomem pointer for pci and none-pci devices. libata core layer _never_ touches those device specific stuff. Using iomap directly is just a convenient way to access the address without adding host private data for PCI controllers. If it doesn't fit, the correct thing to do is to add proper host private data. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Current qc_defer implementation may lead to infinite recursion
Elias Oltmanns wrote:
> +static int piix_qc_defer(struct ata_queued_cmd *qc)
> +{
> + static struct ata_port *ap = NULL;
> + struct ata_queued_cmd qcmd[ATA_MAX_QUEUE];
missing static?
> + static int count = 0;
> +#define PIIX_QC_DEFER_THRESHOLD 5000
> +
> + if (!ap)
> + ap = qc->ap;
> + else if (ap != qc->ap)
> + return 0;
> +
> + if (count > PIIX_QC_DEFER_THRESHOLD + 100)
> + return 0;
> +
> + count++;
> + if (count < PIIX_QC_DEFER_THRESHOLD)
> + return 0;
> + else if (count == PIIX_QC_DEFER_THRESHOLD) {
> + int i;
> + if (ap->qc_allocated) {
> + for (i = 0; i < ATA_MAX_QUEUE; i++)
> + qcmd[i] = ap->qcmd[i];
> + printk(KERN_DEBUG "piix_qc_defer(): saved current
> state\n");
> + msleep(5000);
> + } else
> + count--;
> + } else if (count == PIIX_QC_DEFER_THRESHOLD + 100) {
> + dump_stack();
> + count++;
> + } else if (memcmp(qcmd, ap->qcmd, sizeof(qcmd[0]) * ATA_MAX_QUEUE))
memcmp() will always mismatch. qcmd contains garbage.
> + return ATA_DEFER_LINK;
> + else
> + count = 0;
> + return 0;
> +}
> +
> static const struct piix_map_db ich5_map_db = {
> .mask = 0x7,
> .port_enable = 0x3,
> ---
>
>
>
> Feb 11 20:33:05 denkblock kernel: piix_qc_defer(): saved current state
> Feb 11 20:33:11 denkblock kernel: BUG: scheduling while atomic:
> swapper/0/0x0100
You can't call msleep(5000) there.
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH #upstream] libata: implement libata.force module parameter
Tejun Heo wrote:
> Sam Ravnborg wrote:
>> I have lost the actual patch.
>> But what you see is what happens when you mix const and non-const data
>> in the same section.
>>
>> Look for use of __initdata for const data and replace it with __initconst.
>>
>> And modpost cannot warn about it as gcc errors out before we look at the
>> .o file with modpost.
>
> OIC, thanks. Hmmm... in init.h, I see __{dev|cpu|mem}initconst but no
> __initconst. The data structure in question is used from module init
> function tagged properly with __init. What should be done here?
PING.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system
Holger Macht wrote: > On a related note, shouldn't ata_acpi_handle_hotplug delete the device > like what is done when doing > > echo 1 > /sys/devices/.../block/sr0/device/delete Yeap, when the ata_acpi_handle_hotplug() was added, the focus was supporting hotplug when the controller itself doesn't have hotplug notification. Removing device for undocking would need explicitly killing it. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system
Holger Macht wrote:
>> It should be called via ata_acpi_{ap|dev}_notify() callbacks installed
>> via acpi_install_notify_handler(). Can you add dump_stack() in the
>> function and verify that it actually is being called? It could be that
>> the method is called too late or libata takes too long to actually
>> unplug the device. Hmmm... It seems what ata_acpi_handle_hotplug() does
>> isn't enough for undock. It probably should request detaching the
>> device instead of just notifying hotplug event. Anyways, please lemme
>> know whether and when the function is called.
>
> I already checked, it's never called AFAICS. And I couldn't find a place
> where it should be installed, otherwise, I would have sent a patch. The
> dock driver already calls the notify methods on devices in the dock
> station before doing the real undock.
ata_acpi_associate() calls acpi_install_notify_handler() for each
device. Isn't that enough?
> immediate_undock=1:
> User presses undock button on the dock station, dock driver calls ACPI
> undock method immediately.
>
> immediate_undock=0:
> User presses undock button on the dock station, dock driver throws uevent
> and waits for userland to undock the system via sysfs.
>
> immediate_undock is currently set to 1 by default.
Thanks for the explanation.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system
Hello,
Holger Macht wrote:
>> In the above example, even the reset sequence itself can cause hang if
>> the hardware is implemented slightly differently. The reason why
>> set_piomode() locks up but reset sequence doesn't is simple dumb luck.
>> I think the proper fix is to tell libata to detach the cdrom before
>> undocking.
>
> Wouldn't the proper fix be to call ata_acpi_handle_hotplug _somewhere_?
> (which is currently called nowhere AFAICS)
It should be called via ata_acpi_{ap|dev}_notify() callbacks installed
via acpi_install_notify_handler(). Can you add dump_stack() in the
function and verify that it actually is being called? It could be that
the method is called too late or libata takes too long to actually
unplug the device. Hmmm... It seems what ata_acpi_handle_hotplug() does
isn't enough for undock. It probably should request detaching the
device instead of just notifying hotplug event. Anyways, please lemme
know whether and when the function is called.
> Anyway, kernel hackers keep telling me that the kernel should just do the
> right thing. Shouldn't userspace never be able to freeze the system?
Yeah, I think most things should be done automatically but it's true
that somethings are a bit awkward to handle in kernel. Also, if you're
root, you can almost always crash the machine from userland.
> It's completely ok for me to handle this from userspace, if that's the
> position of the libata developers.
Let's see whether we can fix the ACPI handler first.
> In this case, we should change the dock driver to default to
> immediate_undock=false, because otherwise it's far too risky to freeze the
> system.
I'm not too familiar with how docks work. Can you please explain
briefly what immediate_undock is?
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system
Hello, Holger Macht wrote: >> In the above example, even the reset sequence itself can cause hang if >> the hardware is implemented slightly differently. The reason why >> set_piomode() locks up but reset sequence doesn't is simple dumb luck. > > Another thing, whether it's poor luck or not, it worked like this than at > least 2.6.22. And it was _heavily_ tested. The above commit broke it > between 2.6.24-rc1 and 2.6.24-rc2, which is at least a regression. Yes, in a sense but the change is in the area where there's no defined behavior, so I don't think it's a good idea to revert the change here. Let's concentrate on finding the proper solution. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Current qc_defer implementation may lead to infinite recursion
Hello, Elias Oltmanns wrote: >> Hmmm... The reason why max_host_blocked and max_device_blocked are set >> to 1 is to let libata re-consider status after each command completion >> as blocked status can be rather complex w/ PMP. I haven't really >> followed the code yet but you're saying that blocked count of 2 should >> be used for that behavior, right? > > Not quite. On an SMP system the current implementation will probably do > exactly what you had in mind. In particular, setting max_device_blocked > and max_host_blocked to 1 seems to be the right thing to do in this > case. I currently can't test the code but SMP or not doesn't make much difference if your analysis is correct. You're saying the the code will infinitely recurse if qc_defer returns non-zero but on SMP the recursion will be terminated by another CPU finishing a pending request, right? I don't think things will fan out that way. The recursing thread will have much more than enough time to overflow the stack before any IO event comes to stop the recursion. I don't think such infinite recursions can happen because blk_run_queue() doesn't allow recursing more than once. >> Another strange thing is that there hasn't been any such lock up / >> infinite recursion report till now although ->qc_defer mechanism bas >> been used widely for some time now. Can you reproduce the problem w/o >> the disk shock protection? > > No, unfortunately, I'm unable to reproduce this without the patch on my > machine. This is for purely technical reasons though because I'm using > ata_piix. Running a vanilla kernel, I'd expect everything to work just > fine except for one case: A non-SMP system using a driver that provides > the ->qc_defer() callback. Currently, the ->qc_defer() callback is the > only thing that can possibly send a non zero return value to the scsi > midlayer. Once it does, however, the driver will only get a chance to > complete some qcs before ->qc_defer() is called again provided that > multithreading is supported. > > So, what I'm saying is this: If the low level driver doesn't provide a > ->qc_defer() callback, there is no (obvious) reason why > max_device_blocked and max_host_blocked should be set to 1 since libata > won't gain anything by it. However, it is not a bug either, even though > James considers it suboptimal and I will have to think about a solution > for my patch. On the other hand, once a driver defines the ->qc_defer() > callback, we really have a bug because things will go wrong once > ->qc_defer() returns non zero on a uniprocessor. So, in this case > max_device_blocked and max_host_blocked should be set to 1 on an SMP > system and *have to* be bigger than 1 otherwise. Well, we need to support ->qc_defer on UP systems too. I'm still very skeptical that the scenario you described can happen. The mechanism is just used too widely for such problem to go unnoticed. For example, deferring is exercised when FLUSH is issued to a driver which have in-flight NCQ commands. If you have barrier enabled FS mounted on an NCQ enabled hard drive, such events will happen regularly but we haven't heard of any similar lock up or stack overflow till now. Feel free to add ->qc_defer to ata_piix and let it defer commands (say every 20th or any other criteria you seem fit for testing) but I think the worst can happen is somewhat inefficient deferring sequence like the following. 1. ->qc_defer returns DEVICE BUSY 2. device_blocked set to 1 3. scsi_queue_insert() ends up calling blk_run_queue() 4. blk_run_queue() recurses, device_blocked cleared and queue processing starts again. 5. ->qc_defer returns DEVICE BUSY again 6. device_blocked set to 1 7. scsi_queue_insert() ends up calling blk_run_queue() 8. blk_run_queue() detects it's recursing the second time and veils. At this point, although it went through unnecessary loop through queue processing, everything is well. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Current qc_defer implementation may lead to infinite recursion
Elias Oltmanns wrote: > Hi Tejun, > > due to your commit 31cc23b34913bc173680bdc87af79e551bf8cc0d libata now > sets max_host_blocked and max_device_blocked to 1 for all devices it > manages. Under certain conditions this may lead to system lockups due to > infinite recursion as I have explained to James on the scsi list (kept > you cc-ed). James told me that it was the business of libata to make > sure that such a recursion cannot happen. > > In my discussion with James I imprudently claimed that this was easy to > fix in libata. However, after giving the matter some thought, I'm not at > all sure as to what exactly should be done about it. The easy bit is > that max_host_blocked and max_device_blocked should be left alone as > long as the low level driver does not provide the ->qc_defer() callback. > But even if the driver has defined this callback, ata_std_qc_defer() for > one will not prevent this recursion on a uniprocessor, whereas things > might work out well on an SMP system due to the lock fiddling in the > scsi midlayer. > > As a conclusion, the current implementation makes it imperative to leave > max_host_blocked and max_device_blocked alone on a uniprocessor system. > For SMP systems the current implementation might just be fine but even > there it might just as well be a good idea to make the adjustment > depending on ->qc_defer != NULL. Hmmm... The reason why max_host_blocked and max_device_blocked are set to 1 is to let libata re-consider status after each command completion as blocked status can be rather complex w/ PMP. I haven't really followed the code yet but you're saying that blocked count of 2 should be used for that behavior, right? Another strange thing is that there hasn't been any such lock up / infinite recursion report till now although ->qc_defer mechanism bas been used widely for some time now. Can you reproduce the problem w/o the disk shock protection? Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] libata: Forcing PIO0 mode on reset must not freeze system
Hello,
Holger Macht wrote:
> Calling ap->ops->set_piomode(ap, dev) on a device/controller which got
> already removed, locks the system hard. Reproducibly on an X60 attached to
> a dock station containing a cdrom device with doing
>
> $ echo 1 > /sys/devices/platform/dock.0/undock && echo 123 > /dev/sr0
>
> This calls ata_eh_reset(...) which in turn tries to force PIO mode 0. But
> the device is already gone.
>
> Bisecting revealed the following commit as culprit:
>
> commit cdeab1140799f09c5f728a5ff85e0bdfa5679cd2
> Author: Tejun Heo <[EMAIL PROTECTED]>
> Date: Mon Oct 29 16:41:09 2007 +0900
>
> libata: relocate forcing PIO0 on reset
>
> Forcing PIO0 on reset was done inside ata_bus_softreset(), which is a
> bit out of place as it should be applied to all resets - hard, soft
> and implementation which don't use ata_bus_softreset(). Relocate it
> such that...
>
> * For new EH, it's done in ata_eh_reset() before calling prereset.
>
> * For old EH, it's done before calling ap->ops->phy_reset() in
> ata_bus_probe().
>
> This makes PIO0 forced after all resets. Another difference is that
> reset itself is done after PIO0 is forced.
>
> Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
> Acked-by: Alan Cox <[EMAIL PROTECTED]>
> Signed-off-by: Jeff Garzik <[EMAIL PROTECTED]>
>
>
> ATTENTION! The following patch solves the problem on my system, but please
> be aware that I don't really know what I'm doing because I don't have the
> big picture. There's surely a better way to check if the device/controller
> is still functional than calling ata_link_{online,offline}.
In the above example, even the reset sequence itself can cause hang if
the hardware is implemented slightly differently. The reason why
set_piomode() locks up but reset sequence doesn't is simple dumb luck.
I think the proper fix is to tell libata to detach the cdrom before
undocking.
> Signed-off-by: Holger Macht <[EMAIL PROTECTED]>
NACK.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] libata: normalize port_info, port_operations and sht tables
Tejun Heo wrote:
>>> * No reason to set ata_pci_default_filter() for PIO-only drivers.
>> and your patches add the calls for the CS5520 ?
>>
>>> diff --git a/drivers/ata/pata_cs5520.c b/drivers/ata/pata_cs5520.c
>>> index 972ed9f..5614e76 100644
>>> --- a/drivers/ata/pata_cs5520.c
>>> +++ b/drivers/ata/pata_cs5520.c
>>> @@ -160,6 +160,7 @@ static struct scsi_host_template cs5520_sht = {
>>> static struct ata_port_operations cs5520_port_ops = {
>>> .set_piomode= cs5520_set_piomode,
>>> .set_dmamode= cs5520_set_dmamode,
>>> + .mode_filter= ata_pci_default_filter,
>> This case is wrong. CS5520 doesn't do DMA (just VDMA which we don't
>> support) and in addition doesn't use BAR4 so its not a generic PCI
>> controller and this is asking for trouble later.
Hmm... cs5520's vdma looks like BMDMA and behaves like one. BMDMA
methods are used and, although it doesn't use BAR4, ioaddr->bmdma_addr
is initialized, so as long as libata SFF layer is concerned, it can be
considered a BMDMA controller.
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] libata: normalize port_info, port_operations and sht tables
Alan Cox wrote:
>> * Every driver for SFF controllers now uses ata_pci_default_filter()
>> unless the driver has custom implementation.
>
> That is only needed for DMA capable devices. I guess it does no harm to
> be consistent and call it anyway but you then say ..
Yeah, it's kind of fuzzy to distinguish SFF and BMDMA functions and we
mix the names. I'll clean up the names afterwards. Things like that
are much easier after this patchset.
>> * No reason to set ata_pci_default_filter() for PIO-only drivers.
>
> and your patches add the calls for the CS5520 ?
>
>> diff --git a/drivers/ata/pata_cs5520.c b/drivers/ata/pata_cs5520.c
>> index 972ed9f..5614e76 100644
>> --- a/drivers/ata/pata_cs5520.c
>> +++ b/drivers/ata/pata_cs5520.c
>> @@ -160,6 +160,7 @@ static struct scsi_host_template cs5520_sht = {
>> static struct ata_port_operations cs5520_port_ops = {
>> .set_piomode= cs5520_set_piomode,
>> .set_dmamode= cs5520_set_dmamode,
>> +.mode_filter= ata_pci_default_filter,
>
> This case is wrong. CS5520 doesn't do DMA (just VDMA which we don't
> support) and in addition doesn't use BAR4 so its not a generic PCI
> controller and this is asking for trouble later.
Ah.. okay. Thanks for pointing out.
>> diff --git a/drivers/ata/pata_opti.c b/drivers/ata/pata_opti.c
>> index 8f79447..ebb9dc1 100644
>> --- a/drivers/ata/pata_opti.c
>> +++ b/drivers/ata/pata_opti.c
>> @@ -184,6 +184,7 @@ static struct scsi_host_template opti_sht = {
>>
>> static struct ata_port_operations opti_port_ops = {
>> .set_piomode= opti_set_piomode,
>> +.mode_filter= ata_pci_default_filter,
>
> PIO only
>
>
>> --- a/drivers/ata/pata_rz1000.c
>> +++ b/drivers/ata/pata_rz1000.c
>> @@ -72,6 +72,7 @@ static struct scsi_host_template rz1000_sht = {
>>
>> static struct ata_port_operations rz1000_port_ops = {
>> .set_mode = rz1000_set_mode,
>> +.mode_filter= ata_pci_default_filter,
>
> PIO only
Will update accordingly. Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] libata: stop overloading port_info->private_data
Alan Cox wrote: > On Wed, 30 Jan 2008 18:29:01 +0900 > Tejun Heo <[EMAIL PROTECTED]> wrote: > >> port_info->private_data is currently used for two purposes - to record >> private data about the port_info or to specify host->private_data to >> use when allocating ata_host. > > Sensible point, horrible implementation - I'd NAK this strongly in favour > of switching to: > > ata_pci_init_one(., private_ptr); > > otherwise when that glue changes we are going to keep having to fix six > or more drivers. That alloc-init-register sequence is now the standard initialization sequence used by all drivers which either aren't SFF or need extra stuff on top of SFF. ata_pci_init_one() being pretty simple at this point, I thought the converted ones could just follow the suit instead of updating every ata_pci_init_one() call. Oh well, you deal with most SFF drivers anyway. I'll add "void *host_priv" to ata_pci_init_one(). Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] libata: normalize port_info, port_operations and sht tables
Jeff Garzik wrote:
> Tejun Heo wrote:
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 07dcaf6..08301ca 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -351,6 +351,8 @@ static const struct ata_port_operations
>> ahci_vt8251_ops = {
>> .port_suspend= ahci_port_suspend,
>> .port_resume= ahci_port_resume,
>> #endif
>> +.enable_pm= ahci_enable_alpm,
>> +.disable_pm= ahci_disable_alpm,
>>
>> .port_start= ahci_port_start,
>> .port_stop= ahci_port_stop,
>> @@ -385,6 +387,8 @@ static const struct ata_port_operations
>> ahci_p5wdh_ops = {
>> .port_suspend= ahci_port_suspend,
>> .port_resume= ahci_port_resume,
>> #endif
>> +.enable_pm= ahci_enable_alpm,
>> +.disable_pm= ahci_disable_alpm,
>>
>> .port_start= ahci_port_start,
>> .port_stop= ahci_port_stop,
>
> The last one is probably OK, but I didn't think vt8521 could do this?
I don't know. Those should be determined by CAP bits and if vt8251 lies
about it, it should be blacklisted by clearing CAP bit as other features.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET libata-dev#upstream] clean up scsi_host_templates and ata_port_operations
Akira Iguchi wrote: > Tejun Heo wrote: >> The following drivers need specific platform to build, so they need >> verification. If you work on one of the following drivers, please >> verify that the driver builds and works fine. It would be best if you >> can verify that the sht and ops don't change by the fifth path using >> the method I'll write in another message. > .. >> * pata_scc > > I check pata_scc and it works fine. > Your verification method detects 1 difference in ops > (.thaw: NULL -> ata_bmdma_thaw) but there is no problem. Thanks. I'll fix that up. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] libata: eliminate the home grown dma padding in favour of that provided by the block layer
From: James Bottomley <[EMAIL PROTECTED]>
ATA requires that all DMA transfers begin and end on word boundaries.
Because of this, a large amount of machinery grew up in ide to adjust
scatterlists on this basis. However, as of 2.5, the block layer has a
dma_alignment variable which ensures both the beginning and length of a
DMA transfer are aligned on the dma_alignment boundary. Although the
block layer does adjust the beginning of the transfer to ensure this
happens, it doesn't actually adjust the length, it merely makes sure
that space is allocated for transfers beyond the declared length. The
upshot of this is that scatterlists may be padded to any size between
the actual length and the length adjusted to the dma_alignment safely
knowing that memory is allocated in this region.
Right at the moment, SCSI takes the default dma_aligment which is on a
512 byte boundary. Note that this aligment only applies to transfers
coming in from user space. However, since all kernel allocations are
automatically aligned on a minimum of 32 byte boundaries, it is safe to
adjust them in this manner as well.
tj: * Adjusting sg after padding is done in block layer. Make libata
set queue alignment correctly for ATAPI devices and drop broken
sg mangling from ata_sg_setup().
* Use request->raw_data_len for ATAPI transfer chunk size.
* Killed qc->raw_nbytes.
* Separated out killing qc->n_iter.
Signed-off-by: James Bottomley <[EMAIL PROTECTED]>
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/ahci.c|5 --
drivers/ata/libata-core.c | 145 +++--
drivers/ata/libata-scsi.c | 23 ++-
drivers/ata/pata_icside.c |8 --
drivers/ata/sata_fsl.c| 13
drivers/ata/sata_mv.c |6 +--
drivers/ata/sata_sil24.c |5 --
drivers/scsi/ipr.c|4 +-
drivers/scsi/libsas/sas_ata.c |4 +-
include/linux/libata.h| 28 +
10 files changed, 21 insertions(+), 220 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 29e71bd..3c06e45 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1975,16 +1975,11 @@ static int ahci_port_start(struct ata_port *ap)
struct ahci_port_priv *pp;
void *mem;
dma_addr_t mem_dma;
- int rc;
pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
if (!pp)
return -ENOMEM;
- rc = ata_pad_alloc(ap, dev);
- if (rc)
- return rc;
-
mem = dmam_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma,
GFP_KERNEL);
if (!mem)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ecb8275..d49b271 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4474,30 +4474,13 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
struct ata_port *ap = qc->ap;
struct scatterlist *sg = qc->sg;
int dir = qc->dma_dir;
- void *pad_buf = NULL;
WARN_ON(sg == NULL);
- VPRINTK("unmapping %u sg elements\n", qc->mapped_n_elem);
+ VPRINTK("unmapping %u sg elements\n", qc->n_elem);
- /* if we padded the buffer out to 32-bit bound, and data
-* xfer direction is from-device, we must copy from the
-* pad buffer back into the supplied buffer
-*/
- if (qc->pad_len && !(qc->tf.flags & ATA_TFLAG_WRITE))
- pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
-
- if (qc->mapped_n_elem)
- dma_unmap_sg(ap->dev, sg, qc->mapped_n_elem, dir);
- /* restore last sg */
- if (qc->last_sg)
- *qc->last_sg = qc->saved_last_sg;
- if (pad_buf) {
- struct scatterlist *psg = &qc->extra_sg[1];
- void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
- memcpy(addr + psg->offset, pad_buf, qc->pad_len);
- kunmap_atomic(addr, KM_IRQ0);
- }
+ if (qc->n_elem)
+ dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
qc->flags &= ~ATA_QCFLAG_DMAMAP;
qc->sg = NULL;
@@ -4748,97 +4731,6 @@ void ata_sg_init(struct ata_queued_cmd *qc, struct
scatterlist *sg,
qc->cursg = qc->sg;
}
-static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
- unsigned int *n_elem_extra,
- unsigned int *nbytes_extra)
-{
- struct ata_port *ap = qc->ap;
- unsigned int n_elem = qc->n_elem;
- struct scatterlist *lsg, *copy_lsg = NULL, *tsg = NULL, *esg = NULL;
-
- *n_elem_extra = 0;
- *nbytes_extra = 0;
-
- /* needs padding? */
- qc->pad_len = qc->nbytes & 3;
-
- if (likely(!qc->pad_len))
- return n_el
[PATCH 7/7] libata: implement drain buffers
From: James Bottomley <[EMAIL PROTECTED]>
This just updates the libata slave configure routine to take advantage
of the block layer drain buffers. It also adjusts the size lengths in
the atapi code to add the drain buffer to the DMA length so the driver
knows it can rely on it.
I suspect I should also be checking for AHCI as well as ATA_DEV_ATAPI,
but I couldn't see how to do that easily.
tj: * atapi_drain_needed() added such that draining is applied to only
misc ATAPI commands.
* q->bounce_gfp used when allocating drain buffer.
* Now duplicate ATAPI PIO drain logic dropped.
* ata_dev_printk() used instead of sdev_printk().
Signed-off-by: James Bottomley <[EMAIL PROTECTED]>
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/libata-core.c | 56 +++---
drivers/ata/libata-scsi.c | 59
2 files changed, 57 insertions(+), 58 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d49b271..c03cef4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4623,28 +4623,6 @@ int ata_check_atapi_dma(struct ata_queued_cmd *qc)
}
/**
- * atapi_qc_may_overflow - Check whether data transfer may overflow
- * @qc: ATA command in question
- *
- * ATAPI commands which transfer variable length data to host
- * might overflow due to application error or hardare bug. This
- * function checks whether overflow should be drained and ignored
- * for @qc.
- *
- * LOCKING:
- * None.
- *
- * RETURNS:
- * 1 if @qc may overflow; otherwise, 0.
- */
-static int atapi_qc_may_overflow(struct ata_queued_cmd *qc)
-{
- return ata_is_atapi(qc->tf.protocol) && ata_is_data(qc->tf.protocol) &&
- atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC &&
- !(qc->tf.flags & ATA_TFLAG_WRITE);
-}
-
-/**
* ata_std_qc_defer - Check whether a qc needs to be deferred
* @qc: ATA command in question
*
@@ -5007,36 +4985,10 @@ static int __atapi_pio_bytes(struct ata_queued_cmd *qc,
unsigned int bytes)
next_sg:
sg = qc->cursg;
if (unlikely(!sg)) {
- /*
-* The end of qc->sg is reached and the device expects
-* more data to transfer. In order not to overrun qc->sg
-* and fulfill length specified in the byte count register,
-*- for read case, discard trailing data from the device
-*- for write case, padding zero data to the device
-*/
- u16 pad_buf[1] = { 0 };
-
- if (qc->curbytes + bytes > qc->nbytes + ATAPI_MAX_DRAIN) {
- ata_ehi_push_desc(ehi, "too much trailing data "
- "buf=%u cur=%u bytes=%u",
- qc->nbytes, qc->curbytes, bytes);
- return -1;
- }
-
- /* allow overflow only for misc ATAPI commands */
- if (!atapi_qc_may_overflow(qc)) {
- ata_ehi_push_desc(ehi, "unexpected trailing data "
- "%u bytes", bytes);
- return -1;
- }
-
- consumed = 0;
- while (consumed < bytes)
- consumed += ap->ops->data_xfer(dev,
- (unsigned char *)pad_buf, 2, rw);
-
- qc->curbytes += bytes;
- return 0;
+ ata_ehi_push_desc(ehi, "unexpected or too much trailing data "
+ "buf=%u cur=%u bytes=%u",
+ qc->nbytes, qc->curbytes, bytes);
+ return -1;
}
page = sg_page(sg);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e54ee6e..b3b3b44 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -826,17 +826,56 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
sdev->max_device_blocked = 1;
}
-static void ata_scsi_dev_config(struct scsi_device *sdev,
- struct ata_device *dev)
+/**
+ * atapi_drain_needed - Check whether data transfer may overflow
+ * @request: request to be checked
+ *
+ * ATAPI commands which transfer variable length data to host
+ * might overflow due to application error or hardare bug. This
+ * function checks whether overflow should be drained and ignored
+ * for @request.
+ *
+ * LOCKING:
+ * None.
+ *
+ * RETURNS:
+ * 1 if ; otherwise, 0.
+ */
+static int atapi_drain_needed(struct request *rq)
+{
+ if (likely(!blk_pc_request(rq)))
+ return 0;
+
+ if (
[PATCH 5/7] block: clear drain buffer if draining for write command
Clear drain buffer before chaining if the command in question is a
write.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
block/blk-merge.c |3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index d50cfc8..d0b9031 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -221,6 +221,9 @@ new_segment:
} /* segments in rq */
if (q->dma_drain_size && q->dma_drain_needed(rq)) {
+ if (rq->cmd_flags & REQ_RW)
+ memset(q->dma_drain_buffer, 0, q->dma_drain_size);
+
sg->page_link &= ~0x02;
sg = sg_next(sg);
sg_set_page(sg, virt_to_page(q->dma_drain_buffer),
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] libata: update ATAPI overflow draining
For misc ATAPI commands which transfer variable length data to the
host, overflow can occur due to application or hardware bug. Such
overflows can be ignored safely as long as overflow data is properly
drained. libata HSM implementation has this implemented in
__atapi_pio_bytes() and recently updated for 2.6.24-rc but it requires
further improvements. Improve drain logic such that...
* Report overflow errors using ehi desc mechanism instead of printing
directly.
* Properly calculate the number of bytes to be drained considering
actual number of consumed bytes for partial draining.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Acked-by: Albert Lee <[EMAIL PROTECTED]>
---
drivers/ata/libata-core.c | 76 +++-
1 files changed, 33 insertions(+), 43 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3011919..ecb8275 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4656,24 +4656,9 @@ int ata_check_atapi_dma(struct ata_queued_cmd *qc)
*/
static int atapi_qc_may_overflow(struct ata_queued_cmd *qc)
{
- if (qc->tf.protocol != ATAPI_PROT_PIO &&
- qc->tf.protocol != ATAPI_PROT_DMA)
- return 0;
-
- if (qc->tf.flags & ATA_TFLAG_WRITE)
- return 0;
-
- switch (qc->cdb[0]) {
- case READ_10:
- case READ_12:
- case WRITE_10:
- case WRITE_12:
- case GPCMD_READ_CD:
- case GPCMD_READ_CD_MSF:
- return 0;
- }
-
- return 1;
+ return ata_is_atapi(qc->tf.protocol) && ata_is_data(qc->tf.protocol) &&
+ atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC &&
+ !(qc->tf.flags & ATA_TFLAG_WRITE);
}
/**
@@ -5127,13 +5112,14 @@ static void atapi_send_cdb(struct ata_port *ap, struct
ata_queued_cmd *qc)
*/
static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
{
- int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
+ int rw = (qc->tf.flags & ATA_TFLAG_WRITE) ? WRITE : READ;
struct ata_port *ap = qc->ap;
- struct ata_eh_info *ehi = &qc->dev->link->eh_info;
+ struct ata_device *dev = qc->dev;
+ struct ata_eh_info *ehi = &dev->link->eh_info;
struct scatterlist *sg;
struct page *page;
unsigned char *buf;
- unsigned int offset, count;
+ unsigned int offset, count, consumed;
next_sg:
sg = qc->cursg;
@@ -5146,26 +5132,27 @@ next_sg:
*- for write case, padding zero data to the device
*/
u16 pad_buf[1] = { 0 };
- unsigned int i;
- if (bytes > qc->curbytes - qc->nbytes + ATAPI_MAX_DRAIN) {
+ if (qc->curbytes + bytes > qc->nbytes + ATAPI_MAX_DRAIN) {
ata_ehi_push_desc(ehi, "too much trailing data "
"buf=%u cur=%u bytes=%u",
qc->nbytes, qc->curbytes, bytes);
return -1;
}
-/* overflow is exptected for misc ATAPI commands */
- if (bytes && !atapi_qc_may_overflow(qc))
- ata_dev_printk(qc->dev, KERN_WARNING, "ATAPI %u bytes "
- "trailing data (cdb=%02x nbytes=%u)\n",
- bytes, qc->cdb[0], qc->nbytes);
+ /* allow overflow only for misc ATAPI commands */
+ if (!atapi_qc_may_overflow(qc)) {
+ ata_ehi_push_desc(ehi, "unexpected trailing data "
+ "%u bytes", bytes);
+ return -1;
+ }
- for (i = 0; i < (bytes + 1) / 2; i++)
- ap->ops->data_xfer(qc->dev, (unsigned char *)pad_buf,
2, do_write);
+ consumed = 0;
+ while (consumed < bytes)
+ consumed += ap->ops->data_xfer(dev,
+ (unsigned char *)pad_buf, 2, rw);
qc->curbytes += bytes;
-
return 0;
}
@@ -5192,18 +5179,16 @@ next_sg:
buf = kmap_atomic(page, KM_IRQ0);
/* do the actual data transfer */
- ap->ops->data_xfer(qc->dev, buf + offset, count, do_write);
+ consumed = ap->ops->data_xfer(dev, buf + offset, count, rw);
kunmap_atomic(buf, KM_IRQ0);
local_irq_restore(flags);
} else {
buf = page_address(page);
- ap->ops->data_xfer(qc->dev, buf + offset, count, do_write);
+ consumed
[PATCH 3/7] block: add request->raw_data_len
With padding and draining moved into it, block layer now may extend
requests as directed by queue parameters, so now a request has two
sizes - the original request size and the extended size which matches
the size of area pointed to by bios and later by sgs. The latter size
is what lower layers are primarily interested in when allocating,
filling up DMA tables and setting up the controller.
Both padding and draining extend the data area to accomodate
controller characteristics. As any controller which speaks SCSI can
handle underflows, feeding larger data area is safe.
So, this patch makes the primary data length field, request->data_len,
indicate the size of full data area and add a separate length field,
request->raw_data_len, for the unmodified request size. The latter is
used to report to higher layer (userland) and where the original
request size should be fed to the controller or device.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
block/blk-core.c|2 ++
block/blk-map.c |2 ++
block/blk-merge.c |1 +
block/bsg.c |8
block/scsi_ioctl.c |3 ++-
drivers/scsi/scsi_lib.c |8
include/linux/blkdev.h |1 +
7 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 4afb39c..8b004e1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -116,6 +116,7 @@ void rq_init(struct request_queue *q, struct request *rq)
rq->ref_count = 1;
rq->q = q;
rq->special = NULL;
+ rq->raw_data_len = 0;
rq->data_len = 0;
rq->data = NULL;
rq->nr_phys_segments = 0;
@@ -1982,6 +1983,7 @@ void blk_rq_bio_prep(struct request_queue *q, struct
request *rq,
rq->hard_cur_sectors = rq->current_nr_sectors;
rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
rq->buffer = bio_data(bio);
+ rq->raw_data_len = bio->bi_size;
rq->data_len = bio->bi_size;
rq->bio = rq->biotail = bio;
diff --git a/block/blk-map.c b/block/blk-map.c
index 103b1df..1588ea3 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -19,6 +19,7 @@ int blk_rq_append_bio(struct request_queue *q, struct request
*rq,
rq->biotail->bi_next = bio;
rq->biotail = bio;
+ rq->raw_data_len += bio->bi_size;
rq->data_len += bio->bi_size;
}
return 0;
@@ -154,6 +155,7 @@ int blk_rq_map_user(struct request_queue *q, struct request
*rq,
bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
bio->bi_size += pad_len;
+ rq->data_len += pad_len;
}
rq->buffer = rq->data = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 845ef81..480d2bc 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -228,6 +228,7 @@ new_segment:
((unsigned long)q->dma_drain_buffer) &
(PAGE_SIZE - 1));
nsegs++;
+ rq->data_len += q->dma_drain_size;
}
if (sg)
diff --git a/block/bsg.c b/block/bsg.c
index 8917c51..7f3c095 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq,
struct sg_io_v4 *hdr,
}
if (rq->next_rq) {
- hdr->dout_resid = rq->data_len;
- hdr->din_resid = rq->next_rq->data_len;
+ hdr->dout_resid = rq->raw_data_len;
+ hdr->din_resid = rq->next_rq->raw_data_len;
blk_rq_unmap_user(bidi_bio);
blk_put_request(rq->next_rq);
} else if (rq_data_dir(rq) == READ)
- hdr->din_resid = rq->data_len;
+ hdr->din_resid = rq->raw_data_len;
else
- hdr->dout_resid = rq->data_len;
+ hdr->dout_resid = rq->raw_data_len;
/*
* If the request generated a negative error number, return it
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 9675b34..e993cac 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct
sg_io_hdr *hdr,
hdr->info = 0;
if (hdr->masked_status || hdr->host_status || hdr->driver_status)
hdr->info |= SG_INFO_CHECK;
- hdr->resid = rq->data_len;
+ hdr->resid = rq->raw_data_len;
hdr->sb_len_wr = 0;
if (rq->sense_len && hdr->sbp) {
@@ -528,6 +528,7 @@ static int __blk_send_generic(struct request_queue *q,
struct gendisk *bd_disk,
rq = blk_get_request(q, WRITE, __GFP_WAIT);
rq->cmd_type = REQ_TYPE_BLOCK_PC;
rq->da
[PATCHSET #upstream] block/libata: update and use block layer padding and draining, take 2
This is the second take of blk-layer-padding-and-draining patchset. Changes from the last take[L] are... * libata-update-ATAPI-overflow-draining patch added. This patch fixes up ATAPI PIO handling before updating it to use block layer draining. consumed accounting and atapi_qc_may_overflow() update will survive blk draining conversion. * blk-clear-drain-buffer-for-writes patch added. This patch makes block layer clear drain buffer before chaining it to the sg list if the command in question is a write. * libata-implement-drain-buffers updated such that atapi_drain_needed() uses the same logic as atapi_qc_may_overflow() and ATAPI PIO drain logic is replaced too. This patchset is against the current upstream (bc5468f52b785ffa1fe0ea289baec2c51384d436). Thanks. -- tejun [L] http://thread.gmane.org/gmane.linux.ide/28048 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] block: implement request_queue->dma_drain_needed
Draining shouldn't be done for commands where overflow may indicate
data integrity issues. Add dma_drain_needed callback to
request_queue. Drain buffer is appened iff this function returns
non-zero.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
block/blk-merge.c |2 +-
block/blk-settings.c |7 +--
include/linux/blkdev.h |7 +--
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 480d2bc..d50cfc8 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -220,7 +220,7 @@ new_segment:
bvprv = bvec;
} /* segments in rq */
- if (q->dma_drain_size) {
+ if (q->dma_drain_size && q->dma_drain_needed(rq)) {
sg->page_link &= ~0x02;
sg = sg_next(sg);
sg_set_page(sg, virt_to_page(q->dma_drain_buffer),
diff --git a/block/blk-settings.c b/block/blk-settings.c
index c8d0c57..0a0b3a4 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -296,6 +296,7 @@ EXPORT_SYMBOL(blk_queue_stack_limits);
* blk_queue_dma_drain - Set up a drain buffer for excess dma.
*
* @q: the request queue for the device
+ * @dma_drain_needed: fn which returns non-zero if drain is necessary
* @buf: physically contiguous buffer
* @size: size of the buffer in bytes
*
@@ -315,14 +316,16 @@ EXPORT_SYMBOL(blk_queue_stack_limits);
* device can support otherwise there won't be room for the drain
* buffer.
*/
-int blk_queue_dma_drain(struct request_queue *q, void *buf,
- unsigned int size)
+extern int blk_queue_dma_drain(struct request_queue *q,
+ dma_drain_needed_fn *dma_drain_needed,
+ void *buf, unsigned int size)
{
if (q->max_hw_segments < 2 || q->max_phys_segments < 2)
return -EINVAL;
/* make room for appending the drain */
--q->max_hw_segments;
--q->max_phys_segments;
+ q->dma_drain_needed = dma_drain_needed;
q->dma_drain_buffer = buf;
q->dma_drain_size = size;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ee0b021..3912c5d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -257,6 +257,7 @@ struct bio_vec;
typedef int (merge_bvec_fn) (struct request_queue *, struct bio *, struct
bio_vec *);
typedef void (prepare_flush_fn) (struct request_queue *, struct request *);
typedef void (softirq_done_fn)(struct request *);
+typedef int (dma_drain_needed_fn)(struct request *);
enum blk_queue_state {
Queue_down,
@@ -293,6 +294,7 @@ struct request_queue
merge_bvec_fn *merge_bvec_fn;
prepare_flush_fn*prepare_flush_fn;
softirq_done_fn *softirq_done_fn;
+ dma_drain_needed_fn *dma_drain_needed;
/*
* Dispatch queue sorting
@@ -697,8 +699,9 @@ extern void blk_queue_max_hw_segments(struct request_queue
*, unsigned short);
extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
extern void blk_queue_hardsect_size(struct request_queue *, unsigned short);
extern void blk_queue_stack_limits(struct request_queue *t, struct
request_queue *b);
-extern int blk_queue_dma_drain(struct request_queue *q, void *buf,
- unsigned int size);
+extern int blk_queue_dma_drain(struct request_queue *q,
+ dma_drain_needed_fn *dma_drain_needed,
+ void *buf, unsigned int size);
extern void blk_queue_segment_boundary(struct request_queue *, unsigned long);
extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);
extern void blk_queue_merge_bvec(struct request_queue *, merge_bvec_fn *);
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] block: update bio according to DMA alignment padding
DMA start address and transfer size alignment for PC requests are
achieved using bio_copy_user() instead of bio_map_user(). This works
because bio_copy_user() always uses full pages and block DMA alignment
isn't allowed to go over PAGE_SIZE.
However, the implementation didn't update the last bio of the request
to make this padding visible to lower layers. This patch makes
blk_rq_map_user() extend the last bio such that it includes the
padding area and the size of area pointed to by the request is
properly aligned.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
block/blk-map.c | 17 +
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/block/blk-map.c b/block/blk-map.c
index 955d75c..103b1df 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -139,6 +139,23 @@ int blk_rq_map_user(struct request_queue *q, struct
request *rq,
ubuf += ret;
}
+ /*
+* __blk_rq_map_user() copies the buffers if starting address
+* or length isn't aligned. As the copied buffer is always
+* page aligned, we know that there's enough room for padding.
+* Extend the last bio and update rq->data_len accordingly.
+*
+* On unmap, bio_uncopy_user() will use unmodified
+* bio_map_data pointed to by bio->bi_private.
+*/
+ if (len & queue_dma_alignment(q)) {
+ unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
+ struct bio *bio = rq->biotail;
+
+ bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
+ bio->bi_size += pad_len;
+ }
+
rq->buffer = rq->data = NULL;
return 0;
unmap_rq:
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET #upstream] block/libata: update and use block layer padding and draining
Jeff Garzik wrote: > Jens Axboe wrote: >> On Fri, Feb 08 2008, Jeff Garzik wrote: >>> Jeff Garzik wrote: >>>> Tejun Heo wrote: >>>>> This patchset updates block layer padding and draining support and >>>>> make libata use it. It's based on James Bottomley's initial work and, >>>>> of the five, the last two patches are from James with some >>>>> modifications. >>>>> >>>>> Please read the following thread for more info. >>>>> >>>>> http://thread.gmane.org/gmane.linux.scsi/37185 >>>>> >>>>> This patchset is on top of >>>>> >>>>> upstream (a6af42fc9a12165136d82206ad52f18c5955ce87) >>>>> + kill-n_iter-and-fix-fsl patch [1] >>>> ACK patchset... lets definitely get these fixes upstream. >>>> >>>> Once Jens is happy, I would prefer the merge the lot upstream, if >>>> that is OK with everyone involved? >>> Jens, ping? >>> >>> It's a bug fix, so it would be nice to get this in soonish. As >>> noted, if all looks good, I would prefer to merge via libata-dev... >> >> I'm ok with it, but lets please merge the block bits through the block >> repo, since they are not trivial. Wont be until the week after next, >> though. > > hmmm, rather than delaying the bug fixes for two weeks, since you're OK > with it we can push upstream now, and apply further fixes if problems > arise during testing? > > I would rather get these fixes out into wide testing sooner rather than > later. I have an updated version. Please standby a bit. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH #upstream] libata: implement libata.force module parameter
Sam Ravnborg wrote:
> I have lost the actual patch.
> But what you see is what happens when you mix const and non-const data
> in the same section.
>
> Look for use of __initdata for const data and replace it with __initconst.
>
> And modpost cannot warn about it as gcc errors out before we look at the
> .o file with modpost.
OIC, thanks. Hmmm... in init.h, I see __{dev|cpu|mem}initconst but no
__initconst. The data structure in question is used from module init
function tagged properly with __init. What should be done here?
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH #upstream-fixes] libata: ignore deverr on SETXFER if mode is configured
Some controllers (VIA CX700) raise device error on SETXFER even after
mode configuration succeeded. Update ata_dev_set_mode() such that
device error is ignored if transfer mode is configured correctly. To
implement this, device is revalidated even after device error on
SETXFER.
This fixes kernel bugzilla bug 8563.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: Alan Cox <[EMAIL PROTECTED]>
---
drivers/ata/libata-core.c | 48 +++---
1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bdbd55a..bcd2860 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3048,6 +3048,8 @@ int ata_down_xfermask_limit(struct ata_device *dev,
unsigned int sel)
static int ata_dev_set_mode(struct ata_device *dev)
{
struct ata_eh_context *ehc = &dev->link->eh_context;
+ const char *dev_err_whine = "";
+ int ign_dev_err = 0;
unsigned int err_mask;
int rc;
@@ -3057,41 +3059,57 @@ static int ata_dev_set_mode(struct ata_device *dev)
err_mask = ata_dev_set_xfermode(dev);
+ if (err_mask & ~AC_ERR_DEV)
+ goto fail;
+
+ /* revalidate */
+ ehc->i.flags |= ATA_EHI_POST_SETMODE;
+ rc = ata_dev_revalidate(dev, ATA_DEV_UNKNOWN, 0);
+ ehc->i.flags &= ~ATA_EHI_POST_SETMODE;
+ if (rc)
+ return rc;
+
/* Old CFA may refuse this command, which is just fine */
if (dev->xfer_shift == ATA_SHIFT_PIO && ata_id_is_cfa(dev->id))
- err_mask &= ~AC_ERR_DEV;
+ ign_dev_err = 1;
/* Some very old devices and some bad newer ones fail any kind of
SET_XFERMODE request but support PIO0-2 timings and no IORDY */
if (dev->xfer_shift == ATA_SHIFT_PIO && !ata_id_has_iordy(dev->id) &&
dev->pio_mode <= XFER_PIO_2)
- err_mask &= ~AC_ERR_DEV;
+ ign_dev_err = 1;
/* Early MWDMA devices do DMA but don't allow DMA mode setting.
Don't fail an MWDMA0 set IFF the device indicates it is in MWDMA0 */
if (dev->xfer_shift == ATA_SHIFT_MWDMA &&
dev->dma_mode == XFER_MW_DMA_0 &&
(dev->id[63] >> 8) & 1)
- err_mask &= ~AC_ERR_DEV;
+ ign_dev_err = 1;
- if (err_mask) {
- ata_dev_printk(dev, KERN_ERR, "failed to set xfermode "
- "(err_mask=0x%x)\n", err_mask);
- return -EIO;
- }
+ /* if the device is actually configured correctly, ignore dev err */
+ if (dev->xfer_mode == ata_xfer_mask2mode(ata_id_xfermask(dev->id)))
+ ign_dev_err = 1;
- ehc->i.flags |= ATA_EHI_POST_SETMODE;
- rc = ata_dev_revalidate(dev, ATA_DEV_UNKNOWN, 0);
- ehc->i.flags &= ~ATA_EHI_POST_SETMODE;
- if (rc)
- return rc;
+ if (err_mask & AC_ERR_DEV) {
+ if (!ign_dev_err)
+ goto fail;
+ else
+ dev_err_whine = " (device error ignored)";
+ }
DPRINTK("xfer_shift=%u, xfer_mode=0x%x\n",
dev->xfer_shift, (int)dev->xfer_mode);
- ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
- ata_mode_string(ata_xfer_mode2mask(dev->xfer_mode)));
+ ata_dev_printk(dev, KERN_INFO, "configured for %s%s\n",
+ ata_mode_string(ata_xfer_mode2mask(dev->xfer_mode)),
+ dev_err_whine);
+
return 0;
+
+ fail:
+ ata_dev_printk(dev, KERN_ERR, "failed to set xfermode "
+ "(err_mask=0x%x)\n", err_mask);
+ return -EIO;
}
/**
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH #upstream-fixes] pata_via: fix SATA cable detection on cx700
The first port of cx700 is SATA. Fix cable detection.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/pata_via.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index 39627ab..d119a68 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -84,6 +84,7 @@ enum {
VIA_BAD_ID = 0x100, /* Has wrong vendor ID (0x1107) */
VIA_BAD_AST = 0x200, /* Don't touch Address Setup Timing */
VIA_NO_ENABLES = 0x400, /* Has no enablebits */
+ VIA_SATA_PATA = 0x800, /* SATA/PATA combined configuration */
};
/*
@@ -100,7 +101,7 @@ static const struct via_isa_bridge {
{ "vx800", PCI_DEVICE_ID_VIA_VX800,0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST },
{ "vt8237s",PCI_DEVICE_ID_VIA_8237S,0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST },
{ "vt8251", PCI_DEVICE_ID_VIA_8251, 0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST },
- { "cx700", PCI_DEVICE_ID_VIA_CX700,0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST },
+ { "cx700", PCI_DEVICE_ID_VIA_CX700,0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST | VIA_SATA_PATA },
{ "vt6410", PCI_DEVICE_ID_VIA_6410, 0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST | VIA_NO_ENABLES},
{ "vt8237a",PCI_DEVICE_ID_VIA_8237A,0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST },
{ "vt8237", PCI_DEVICE_ID_VIA_8237, 0x00, 0x2f, VIA_UDMA_133 |
VIA_BAD_AST },
@@ -172,6 +173,9 @@ static int via_cable_detect(struct ata_port *ap) {
if (via_cable_override(pdev))
return ATA_CBL_PATA40_SHORT;
+ if ((config->flags & VIA_SATA_PATA) && ap->port_no == 0)
+ return ATA_CBL_SATA;
+
/* Early chips are 40 wire */
if ((config->flags & VIA_UDMA) < VIA_UDMA_66)
return ATA_CBL_PATA40;
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: slow resume from s2ram
Brian Keck wrote: > Hello, > > I can't get my otherwise lovely debian/sid Vaio TX17 to resume promptly > from suspend-to-ram. > > Until recently it resumed at all maybe 1 in 4 times from > 's2ram -f -p' in X, always taking several minutes, apparently > due to the hard disk (1.8" Toshiba HDD1544/MK6006GAH). > > Last week I tried 2 things: > > * added /etc/modprobe.d/libata with > options libata noacpi=0 > > * built a kernel + initrd from debian linux-source-2.6.24 with > CONFIG_BLK_DEV_IDEACPI=y > > This boots OK, with ... > >$ cat /sys/module/libata/parameters/noacpi >0 You're not using libata at all. You're using IDE driver instead. Please try libata ata_piix driver instead. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ahci: fix CAP.NP and PI handling
AHCI uses CAP.NP to indicate the number of ports and PI to tell which
ports are enabled. The only requirement is that the number of ports
indicated by CAP.NP should equal or be higher than the number of
enabled ports in PI.
CAP.NP and PI carry duplicate information and there have been some
interesting cases. Some early AHCI controllers didn't set PI at all
and just implement from port 0 to CAP.NP. An ICH8 board which wired
four out of six available ports had 3 (4 ports) for CAP.NP and 0x33
for PI. While ESB2 has less bits set in PI than the value in CAP.NP.
Till now, ahci driver assumed that PI is invalid if it doesn't match
CAP.NP exactly. This violates AHCI standard and the driver ends up
accessing unmimplemented ports on ESB2.
This patch updates CAP.NP and PI handling such that PI can have less
number of bits set than indicated in CAP.NP and the highest port is
determined as the maximum port of what CAP.NP and PI indicate.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: Jan Beulich <[EMAIL PROTECTED]>
---
drivers/ata/ahci.c | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 27c8d56..29e71bd 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -679,24 +679,20 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
/* cross check port_map and cap.n_ports */
if (port_map) {
- u32 tmp_port_map = port_map;
- int n_ports = ahci_nr_ports(cap);
+ int map_ports = 0;
- for (i = 0; i < AHCI_MAX_PORTS && n_ports; i++) {
- if (tmp_port_map & (1 << i)) {
- n_ports--;
- tmp_port_map &= ~(1 << i);
- }
- }
+ for (i = 0; i < AHCI_MAX_PORTS; i++)
+ if (port_map & (1 << i))
+ map_ports++;
- /* If n_ports and port_map are inconsistent, whine and
-* clear port_map and let it be generated from n_ports.
+ /* If PI has more ports than n_ports, whine, clear
+* port_map and let it be generated from n_ports.
*/
- if (n_ports || tmp_port_map) {
+ if (map_ports > ahci_nr_ports(cap)) {
dev_printk(KERN_WARNING, &pdev->dev,
- "nr_ports (%u) and implemented port map "
- "(0x%x) don't match, using nr_ports\n",
- ahci_nr_ports(cap), port_map);
+ "implemented port map (0x%x) contains more "
+ "ports than nr_ports (%u), using nr_ports\n",
+ port_map, ahci_nr_ports(cap));
port_map = 0;
}
}
@@ -2201,7 +2197,7 @@ static int ahci_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
struct device *dev = &pdev->dev;
struct ahci_host_priv *hpriv;
struct ata_host *host;
- int i, rc;
+ int n_ports, i, rc;
VPRINTK("ENTER\n");
@@ -2255,7 +2251,14 @@ static int ahci_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
if (hpriv->cap & HOST_CAP_PMP)
pi.flags |= ATA_FLAG_PMP;
- host = ata_host_alloc_pinfo(&pdev->dev, ppi, fls(hpriv->port_map));
+ /* CAP.NP sometimes indicate the index of the last enabled
+* port, at other times, that of the last possible port, so
+* determining the maximum port number requires looking at
+* both CAP.NP and port_map.
+*/
+ n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
+
+ host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
if (!host)
return -ENOMEM;
host->iomap = pcim_iomap_table(pdev);
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
Jan Beulich wrote:
> It does, in the description for bits 4:0 of the host capabilities register:
> "Number of Ports (NPS)" RO. Hardwired to 5h to indicate support for 6
> ports. Note that the number of ports indicated in this field may be more
> than the number of ports indicated in the PI (ABAR + 0Ch) register."
Oops, you're right, NP can go over PI. Somehow I've been believing it
should match PI. Oh well, that's me being delusional again. :-(
>From ahci 1.1.
Number of Ports (NP): 0's based value indicating the maximum number
of ports supported by the HBA silicon. A maximum of 32 ports can be
supported. A value of Impl. '0h', indicating one port, is the
minimum requirement. Note that the number of ports Spec. indicated
in this field may be more than the number of ports indicated in the
GHC.PI register.
Does the following patch fix the problem?
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index e75966b..39627c7 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -679,24 +679,20 @@ static void ahci_save_initial_config(struct pci_dev *pdev,
/* cross check port_map and cap.n_ports */
if (port_map) {
- u32 tmp_port_map = port_map;
- int n_ports = ahci_nr_ports(cap);
+ int map_ports = 0;
- for (i = 0; i < AHCI_MAX_PORTS && n_ports; i++) {
- if (tmp_port_map & (1 << i)) {
- n_ports--;
- tmp_port_map &= ~(1 << i);
- }
- }
+ for (i = 0; i < AHCI_MAX_PORTS; i++)
+ if (port_map & (1 << i))
+ map_ports++;
- /* If n_ports and port_map are inconsistent, whine and
-* clear port_map and let it be generated from n_ports.
+ /* If PI has more ports than n_ports, whine and clear
+* port_map and let it be generated from n_ports.
*/
- if (n_ports || tmp_port_map) {
+ if (map_ports > ahci_nr_ports(cap)) {
dev_printk(KERN_WARNING, &pdev->dev,
- "nr_ports (%u) and implemented port map "
- "(0x%x) don't match, using nr_ports\n",
- ahci_nr_ports(cap), port_map);
+ "implemented port map (0x%x) contains more "
+ "ports than nr_ports (%u), using nr_ports\n",
+ port_map, ahci_nr_ports(cap));
port_map = 0;
}
}
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AHCI driver preferring nr_ports over port map
Jan Beulich wrote: >> Yes, we can be more smart if necessary. I don't know. The hardware is >> clearly violating the spec which requires those two values to agree. > > So are you saying the ESB2 spec is violating a higher level spec? I know > almost nothing about AHCI, so please forgive that question... n_ports and PI should agree with each other. That's what the ahci spec requires. If ESB2 spec has different opinion about n_ports, it's in violation of AHCI spec but I don't think it explicitly state such things, does it? >> What status values are you seeing? Hardware vendors usually don't get >> n_ports wrong from the start, they probably have forgotten to decrement >> it by one when one of the ports is plugged for some reason. I bet the >> silicon for the port is there and reporting offline PHY, right? > > This is output from our SLE10SP2 kernel, the output is similar for others: > > <6>scsi2 : ahci > <6>ata3: SATA link down (SStatus 0 SControl 300) > <6>scsi3 : ahci > <6>ata4: SATA link down (SStatus 0 SControl 300) > <6>scsi4 : ahci > <6>ata5: SATA link down (SStatus 4 SControl 300) > <6>scsi5 : ahci > <6>ata6: SATA link down (SStatus 0 SControl 0) > > Even the message relating to ata5 seems a little dubious to me, as it's > not in sync with what the other unused ports say (and also not in sync > with what I see on other boxes - SStatus is always 0 for such ports). I'd like to see more output including leading controller detection but yeah, it seems there's no silicon implemented for the last port. The SStatus value 4 indicates that PHY is offline which usually happens when the PHY is turned off from SControl. Hmm... weird. How many ports does the machine actually have? I agree we'll need to adjust PI handling for the controller. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] libata: implement drain buffers
From: James Bottomley <[EMAIL PROTECTED]>
From: James Bottomley <[EMAIL PROTECTED]>
This just updates the libata slave configure routine to take advantage
of the block layer drain buffers. It also adjusts the size lengths in
the atapi code to add the drain buffer to the DMA length so the driver
knows it can rely on it.
I suspect I should also be checking for AHCI as well as ATA_DEV_ATAPI,
but I couldn't see how to do that easily.
tj: * atapi_drain_needed() added such that draining is applied to only
misc ATAPI commands.
* q->bounce_gfp used when allocating drain buffer.
* ata_dev_printk() used instead of sdev_printk().
Signed-off-by: James Bottomley <[EMAIL PROTECTED]>
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/libata-scsi.c | 41 +++--
1 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e54ee6e..185d699 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -826,17 +826,38 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
sdev->max_device_blocked = 1;
}
-static void ata_scsi_dev_config(struct scsi_device *sdev,
- struct ata_device *dev)
+static int atapi_drain_needed(struct request *rq)
+{
+ if (likely(!blk_pc_request(rq)))
+ return 0;
+
+ return atapi_cmd_type(rq->cmd[0]) == ATAPI_MISC;
+}
+
+static int ata_scsi_dev_config(struct scsi_device *sdev,
+ struct ata_device *dev)
{
/* configure max sectors */
blk_queue_max_sectors(sdev->request_queue, dev->max_sectors);
- if (dev->class == ATA_DEV_ATAPI)
+ if (dev->class == ATA_DEV_ATAPI) {
+ struct request_queue *q = sdev->request_queue;
+ void *buf;
+
/* set the min alignment */
blk_queue_update_dma_alignment(sdev->request_queue,
ATA_DMA_PAD_SZ - 1);
- else {
+
+ /* configure draining */
+ buf = kmalloc(ATAPI_MAX_DRAIN, q->bounce_gfp | GFP_KERNEL);
+ if (!buf) {
+ ata_dev_printk(dev, KERN_ERR,
+ "drain buffer allocation failed\n");
+ return -ENOMEM;
+ }
+
+ blk_queue_dma_drain(q, atapi_drain_needed, buf,
ATAPI_MAX_DRAIN);
+ } else {
/* ATA devices must be sector aligned */
blk_queue_update_dma_alignment(sdev->request_queue,
ATA_SECT_SIZE - 1);
@@ -853,6 +874,8 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
depth = min(ATA_MAX_QUEUE - 1, depth);
scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, depth);
}
+
+ return 0;
}
/**
@@ -871,13 +894,14 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
{
struct ata_port *ap = ata_shost_to_port(sdev->host);
struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
+ int rc = 0;
ata_scsi_sdev_config(sdev);
if (dev)
- ata_scsi_dev_config(sdev, dev);
+ rc = ata_scsi_dev_config(sdev, dev);
- return 0;
+ return rc;
}
/**
@@ -897,6 +921,7 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
void ata_scsi_slave_destroy(struct scsi_device *sdev)
{
struct ata_port *ap = ata_shost_to_port(sdev->host);
+ struct request_queue *q = sdev->request_queue;
unsigned long flags;
struct ata_device *dev;
@@ -912,6 +937,10 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
ata_port_schedule_eh(ap);
}
spin_unlock_irqrestore(ap->lock, flags);
+
+ kfree(q->dma_drain_buffer);
+ q->dma_drain_buffer = NULL;
+ q->dma_drain_size = 0;
}
/**
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] libata: eliminate the home grown dma padding in favour of that provided by the block layer
From: James Bottomley <[EMAIL PROTECTED]>
From: James Bottomley <[EMAIL PROTECTED]>
ATA requires that all DMA transfers begin and end on word boundaries.
Because of this, a large amount of machinery grew up in ide to adjust
scatterlists on this basis. However, as of 2.5, the block layer has a
dma_alignment variable which ensures both the beginning and length of a
DMA transfer are aligned on the dma_alignment boundary. Although the
block layer does adjust the beginning of the transfer to ensure this
happens, it doesn't actually adjust the length, it merely makes sure
that space is allocated for transfers beyond the declared length. The
upshot of this is that scatterlists may be padded to any size between
the actual length and the length adjusted to the dma_alignment safely
knowing that memory is allocated in this region.
Right at the moment, SCSI takes the default dma_aligment which is on a
512 byte boundary. Note that this aligment only applies to transfers
coming in from user space. However, since all kernel allocations are
automatically aligned on a minimum of 32 byte boundaries, it is safe to
adjust them in this manner as well.
tj: * Adjusting sg after padding is done in block layer. Make libata
set queue alignment correctly for ATAPI devices and drop broken
sg mangling from ata_sg_setup().
* Use request->raw_data_len for ATAPI transfer chunk size.
* Killed qc->raw_nbytes.
* Separated out killing qc->n_iter.
Signed-off-by: James Bottomley <[EMAIL PROTECTED]>
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
drivers/ata/ahci.c|5 --
drivers/ata/libata-core.c | 145 +++--
drivers/ata/libata-scsi.c | 23 ++-
drivers/ata/pata_icside.c |8 --
drivers/ata/sata_fsl.c| 13
drivers/ata/sata_mv.c |6 +--
drivers/ata/sata_sil24.c |5 --
drivers/scsi/ipr.c|4 +-
drivers/scsi/libsas/sas_ata.c |4 +-
include/linux/libata.h| 28 +
10 files changed, 21 insertions(+), 220 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 27c8d56..e75966b 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1979,16 +1979,11 @@ static int ahci_port_start(struct ata_port *ap)
struct ahci_port_priv *pp;
void *mem;
dma_addr_t mem_dma;
- int rc;
pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
if (!pp)
return -ENOMEM;
- rc = ata_pad_alloc(ap, dev);
- if (rc)
- return rc;
-
mem = dmam_alloc_coherent(dev, AHCI_PORT_PRIV_DMA_SZ, &mem_dma,
GFP_KERNEL);
if (!mem)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index bdbd55a..490e8d4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4476,30 +4476,13 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
struct ata_port *ap = qc->ap;
struct scatterlist *sg = qc->sg;
int dir = qc->dma_dir;
- void *pad_buf = NULL;
WARN_ON(sg == NULL);
- VPRINTK("unmapping %u sg elements\n", qc->mapped_n_elem);
+ VPRINTK("unmapping %u sg elements\n", qc->n_elem);
- /* if we padded the buffer out to 32-bit bound, and data
-* xfer direction is from-device, we must copy from the
-* pad buffer back into the supplied buffer
-*/
- if (qc->pad_len && !(qc->tf.flags & ATA_TFLAG_WRITE))
- pad_buf = ap->pad + (qc->tag * ATA_DMA_PAD_SZ);
-
- if (qc->mapped_n_elem)
- dma_unmap_sg(ap->dev, sg, qc->mapped_n_elem, dir);
- /* restore last sg */
- if (qc->last_sg)
- *qc->last_sg = qc->saved_last_sg;
- if (pad_buf) {
- struct scatterlist *psg = &qc->extra_sg[1];
- void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
- memcpy(addr + psg->offset, pad_buf, qc->pad_len);
- kunmap_atomic(addr, KM_IRQ0);
- }
+ if (qc->n_elem)
+ dma_unmap_sg(ap->dev, sg, qc->n_elem, dir);
qc->flags &= ~ATA_QCFLAG_DMAMAP;
qc->sg = NULL;
@@ -4765,97 +4748,6 @@ void ata_sg_init(struct ata_queued_cmd *qc, struct
scatterlist *sg,
qc->cursg = qc->sg;
}
-static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
- unsigned int *n_elem_extra,
- unsigned int *nbytes_extra)
-{
- struct ata_port *ap = qc->ap;
- unsigned int n_elem = qc->n_elem;
- struct scatterlist *lsg, *copy_lsg = NULL, *tsg = NULL, *esg = NULL;
-
- *n_elem_extra = 0;
- *nbytes_extra = 0;
-
- /* needs padding? */
- qc->pad_len = qc->nbytes & 3;
-
- if
[PATCH 3/5] block: implement request_queue->dma_drain_needed
Draining shouldn't be done for commands where overflow may indicate
data integrity issues. Add dma_drain_needed callback to
request_queue. Drain buffer is appened iff this function returns
non-zero.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
block/blk-merge.c |2 +-
block/blk-settings.c |7 +--
include/linux/blkdev.h |7 +--
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 480d2bc..d50cfc8 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -220,7 +220,7 @@ new_segment:
bvprv = bvec;
} /* segments in rq */
- if (q->dma_drain_size) {
+ if (q->dma_drain_size && q->dma_drain_needed(rq)) {
sg->page_link &= ~0x02;
sg = sg_next(sg);
sg_set_page(sg, virt_to_page(q->dma_drain_buffer),
diff --git a/block/blk-settings.c b/block/blk-settings.c
index c8d0c57..0a0b3a4 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -296,6 +296,7 @@ EXPORT_SYMBOL(blk_queue_stack_limits);
* blk_queue_dma_drain - Set up a drain buffer for excess dma.
*
* @q: the request queue for the device
+ * @dma_drain_needed: fn which returns non-zero if drain is necessary
* @buf: physically contiguous buffer
* @size: size of the buffer in bytes
*
@@ -315,14 +316,16 @@ EXPORT_SYMBOL(blk_queue_stack_limits);
* device can support otherwise there won't be room for the drain
* buffer.
*/
-int blk_queue_dma_drain(struct request_queue *q, void *buf,
- unsigned int size)
+extern int blk_queue_dma_drain(struct request_queue *q,
+ dma_drain_needed_fn *dma_drain_needed,
+ void *buf, unsigned int size)
{
if (q->max_hw_segments < 2 || q->max_phys_segments < 2)
return -EINVAL;
/* make room for appending the drain */
--q->max_hw_segments;
--q->max_phys_segments;
+ q->dma_drain_needed = dma_drain_needed;
q->dma_drain_buffer = buf;
q->dma_drain_size = size;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ee0b021..3912c5d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -257,6 +257,7 @@ struct bio_vec;
typedef int (merge_bvec_fn) (struct request_queue *, struct bio *, struct
bio_vec *);
typedef void (prepare_flush_fn) (struct request_queue *, struct request *);
typedef void (softirq_done_fn)(struct request *);
+typedef int (dma_drain_needed_fn)(struct request *);
enum blk_queue_state {
Queue_down,
@@ -293,6 +294,7 @@ struct request_queue
merge_bvec_fn *merge_bvec_fn;
prepare_flush_fn*prepare_flush_fn;
softirq_done_fn *softirq_done_fn;
+ dma_drain_needed_fn *dma_drain_needed;
/*
* Dispatch queue sorting
@@ -697,8 +699,9 @@ extern void blk_queue_max_hw_segments(struct request_queue
*, unsigned short);
extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
extern void blk_queue_hardsect_size(struct request_queue *, unsigned short);
extern void blk_queue_stack_limits(struct request_queue *t, struct
request_queue *b);
-extern int blk_queue_dma_drain(struct request_queue *q, void *buf,
- unsigned int size);
+extern int blk_queue_dma_drain(struct request_queue *q,
+ dma_drain_needed_fn *dma_drain_needed,
+ void *buf, unsigned int size);
extern void blk_queue_segment_boundary(struct request_queue *, unsigned long);
extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);
extern void blk_queue_merge_bvec(struct request_queue *, merge_bvec_fn *);
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] block: add request->raw_data_len
With padding and draining moved into it, block layer now may extend
requests as directed by queue parameters, so now a request has two
sizes - the original request size and the extended size which matches
the size of area pointed to by bios and later by sgs. The latter size
is what lower layers are primarily interested in when allocating,
filling up DMA tables and setting up the controller.
Both padding and draining extend the data area to accomodate
controller characteristics. As any controller which speaks SCSI can
handle underflows, feeding larger data area is safe.
So, this patch makes the primary data length field, request->data_len,
indicate the size of full data area and add a separate length field,
request->raw_data_len, for the unmodified request size. The latter is
used to report to higher layer (userland) and where the original
request size should be fed to the controller or device.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
block/blk-core.c|2 ++
block/blk-map.c |2 ++
block/blk-merge.c |1 +
block/bsg.c |8
block/scsi_ioctl.c |3 ++-
drivers/scsi/scsi_lib.c |8
include/linux/blkdev.h |1 +
7 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 4afb39c..8b004e1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -116,6 +116,7 @@ void rq_init(struct request_queue *q, struct request *rq)
rq->ref_count = 1;
rq->q = q;
rq->special = NULL;
+ rq->raw_data_len = 0;
rq->data_len = 0;
rq->data = NULL;
rq->nr_phys_segments = 0;
@@ -1982,6 +1983,7 @@ void blk_rq_bio_prep(struct request_queue *q, struct
request *rq,
rq->hard_cur_sectors = rq->current_nr_sectors;
rq->hard_nr_sectors = rq->nr_sectors = bio_sectors(bio);
rq->buffer = bio_data(bio);
+ rq->raw_data_len = bio->bi_size;
rq->data_len = bio->bi_size;
rq->bio = rq->biotail = bio;
diff --git a/block/blk-map.c b/block/blk-map.c
index 103b1df..1588ea3 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -19,6 +19,7 @@ int blk_rq_append_bio(struct request_queue *q, struct request
*rq,
rq->biotail->bi_next = bio;
rq->biotail = bio;
+ rq->raw_data_len += bio->bi_size;
rq->data_len += bio->bi_size;
}
return 0;
@@ -154,6 +155,7 @@ int blk_rq_map_user(struct request_queue *q, struct request
*rq,
bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
bio->bi_size += pad_len;
+ rq->data_len += pad_len;
}
rq->buffer = rq->data = NULL;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 845ef81..480d2bc 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -228,6 +228,7 @@ new_segment:
((unsigned long)q->dma_drain_buffer) &
(PAGE_SIZE - 1));
nsegs++;
+ rq->data_len += q->dma_drain_size;
}
if (sg)
diff --git a/block/bsg.c b/block/bsg.c
index 8917c51..7f3c095 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -437,14 +437,14 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq,
struct sg_io_v4 *hdr,
}
if (rq->next_rq) {
- hdr->dout_resid = rq->data_len;
- hdr->din_resid = rq->next_rq->data_len;
+ hdr->dout_resid = rq->raw_data_len;
+ hdr->din_resid = rq->next_rq->raw_data_len;
blk_rq_unmap_user(bidi_bio);
blk_put_request(rq->next_rq);
} else if (rq_data_dir(rq) == READ)
- hdr->din_resid = rq->data_len;
+ hdr->din_resid = rq->raw_data_len;
else
- hdr->dout_resid = rq->data_len;
+ hdr->dout_resid = rq->raw_data_len;
/*
* If the request generated a negative error number, return it
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 9675b34..e993cac 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -266,7 +266,7 @@ static int blk_complete_sghdr_rq(struct request *rq, struct
sg_io_hdr *hdr,
hdr->info = 0;
if (hdr->masked_status || hdr->host_status || hdr->driver_status)
hdr->info |= SG_INFO_CHECK;
- hdr->resid = rq->data_len;
+ hdr->resid = rq->raw_data_len;
hdr->sb_len_wr = 0;
if (rq->sense_len && hdr->sbp) {
@@ -528,6 +528,7 @@ static int __blk_send_generic(struct request_queue *q,
struct gendisk *bd_disk,
rq = blk_get_request(q, WRITE, __GFP_WAIT);
rq->cmd_type = REQ_TYPE_BLOCK_PC;
rq->da
[PATCH 1/5] block: update bio according to DMA alignment padding
DMA start address and transfer size alignment for PC requests are
achieved using bio_copy_user() instead of bio_map_user(). This works
because bio_copy_user() always uses full pages and block DMA alignment
isn't allowed to go over PAGE_SIZE.
However, the implementation didn't update the last bio of the request
to make this padding visible to lower layers. This patch makes
blk_rq_map_user() extend the last bio such that it includes the
padding area and the size of area pointed to by the request is
properly aligned.
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
Cc: James Bottomley <[EMAIL PROTECTED]>
---
block/blk-map.c | 17 +
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/block/blk-map.c b/block/blk-map.c
index 955d75c..103b1df 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -139,6 +139,23 @@ int blk_rq_map_user(struct request_queue *q, struct
request *rq,
ubuf += ret;
}
+ /*
+* __blk_rq_map_user() copies the buffers if starting address
+* or length isn't aligned. As the copied buffer is always
+* page aligned, we know that there's enough room for padding.
+* Extend the last bio and update rq->data_len accordingly.
+*
+* On unmap, bio_uncopy_user() will use unmodified
+* bio_map_data pointed to by bio->bi_private.
+*/
+ if (len & queue_dma_alignment(q)) {
+ unsigned int pad_len = (queue_dma_alignment(q) & ~len) + 1;
+ struct bio *bio = rq->biotail;
+
+ bio->bi_io_vec[bio->bi_vcnt - 1].bv_len += pad_len;
+ bio->bi_size += pad_len;
+ }
+
rq->buffer = rq->data = NULL;
return 0;
unmap_rq:
--
1.5.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHSET #upstream] block/libata: update and use block layer padding and draining
This patchset updates block layer padding and draining support and make libata use it. It's based on James Bottomley's initial work and, of the five, the last two patches are from James with some modifications. Please read the following thread for more info. http://thread.gmane.org/gmane.linux.scsi/37185 This patchset is on top of upstream (a6af42fc9a12165136d82206ad52f18c5955ce87) + kill-n_iter-and-fix-fsl patch [1] block/blk-core.c |2 block/blk-map.c | 19 + block/blk-merge.c |3 block/blk-settings.c |7 +- block/bsg.c |8 +- block/scsi_ioctl.c|3 drivers/ata/ahci.c|5 - drivers/ata/libata-core.c | 145 ++ drivers/ata/libata-scsi.c | 54 ++- drivers/ata/pata_icside.c |8 -- drivers/ata/sata_fsl.c| 13 --- drivers/ata/sata_mv.c |6 - drivers/ata/sata_sil24.c |5 - drivers/scsi/ipr.c|4 - drivers/scsi/libsas/sas_ata.c |4 - drivers/scsi/scsi_lib.c |8 +- include/linux/blkdev.h|8 +- include/linux/libata.h| 28 18 files changed, 95 insertions(+), 235 deletions(-) Thanks. -- tejun [1] http://article.gmane.org/gmane.linux.ide/28038 - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
