Switching controller mode in software?
Hello everybody, I have a notebook with the Intel ICH7 chipset (see attached lspci output). Unfortunately, the SATA controller is stuck in non-AHCI mode and the braindead bios doesn't allow me to change that. Is it possible to change the controller to AHCI mode in software, e.g. through ACPI? Thanks, Yours, Florian 00:00.0 0600: 8086:27a0 (rev 03) 00:01.0 0604: 8086:27a1 (rev 03) 00:1b.0 0403: 8086:27d8 (rev 02) 00:1c.0 0604: 8086:27d0 (rev 02) 00:1c.1 0604: 8086:27d2 (rev 02) 00:1c.2 0604: 8086:27d4 (rev 02) 00:1d.0 0c03: 8086:27c8 (rev 02) 00:1d.1 0c03: 8086:27c9 (rev 02) 00:1d.2 0c03: 8086:27ca (rev 02) 00:1d.3 0c03: 8086:27cb (rev 02) 00:1d.7 0c03: 8086:27cc (rev 02) 00:1e.0 0604: 8086:2448 (rev e2) 00:1f.0 0601: 8086:27b9 (rev 02) 00:1f.2 0101: 8086:27c4 (rev 02) 00:1f.3 0c05: 8086:27da (rev 02) 01:00.0 0300: 1002:7149 03:00.0 0280: 8086:4222 (rev 02) 0a:01.0 0805: 1180:0822 (rev 19) 0a:01.1 0880: 1180:0843 (rev 01) 0a:01.2 0880: 1180:0592 (rev 0a) 0a:01.3 0880: 1180:0852 (rev 05) 0a:07.0 0200: 10ec:8139 (rev 10) 0a:09.0 0607: 1524:1410 (rev 01) signature.asc Description: Dies ist ein digital signierter Nachrichtenteil
Re: Freeze after disabling write cache with hdparm -W0 /dev/sda
Tejun Heo wrote: Simon Farnsworth wrote: Tejun Heo wrote: Simon Farnsworth wrote: Tejun Heo wrote: Simon Farnsworth wrote: Just a thought; is it possible to trigger libata EH from userspace? If so, we could write a small utility to disable write cache, then force EH to detect the change. That's automatically done. libata snoops cache on/off and triggers revalidation but you can request manual rescan by echoing - - - to /sys/class/scsi_host/hostX/scan. Would it be worth changing our code to do hdparm -W1 /dev/sda hdparm -W0 /dev/sda, or would this not show anything. The lack of revalidation on some drives is what's worrying me a little here. Revalidation happens after cache property is changed successfully. What happens if you request manual rescan while the drive is not repsponding? The 30 second freeze happens when we request manual rescan. We do revalidate once the freeze is over, though; dmesg follows: Can you post dmesg with printk timestamps turned on? Sorry for the delay in responding; Turning on printk timestamps is enough to fix the problem, and stop the stall from happening. We'll be rebasing to the newest Fedora 7 kernel soon (next 3 months), and I've changed our kernel build process to turn on printk timestamps. If it still happens then, I'll get you a dmesg with timestamps. -- Simon Farnsworth - 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] pata_platform: Fix NULL pointer dereference
pata_platform: Fix NULL pointer dereference The platform-specific structures may leave pdev-dev.platform_data as NULL. Signed-off-by: Magnus Damm [EMAIL PROTECTED] --- Tested on a Solution Engline 7722 Applies to git linux-2.6 a5fcaa210626a79465321e344c91a6a7dc3881fa drivers/ata/pata_platform.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) --- 0001/drivers/ata/pata_platform.c +++ work/drivers/ata/pata_platform.c2007-07-17 15:01:09.0 +0900 @@ -213,8 +213,9 @@ static int __devinit pata_platform_probe pata_platform_setup_port(ap-ioaddr, pp_info); /* activate */ - return ata_host_activate(host, platform_get_irq(pdev, 0), ata_interrupt, -pp_info-irq_flags, pata_platform_sht); + return ata_host_activate(host, platform_get_irq(pdev, 0), +ata_interrupt, pp_info ? pp_info-irq_flags +: 0, pata_platform_sht); } /** - 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: Random freezes with libata + ICH7 PATA
Hi again, If I can be of help with more information, please tell me what you need. Full dmesg including boot log would be a nice place to start. Did the kernel say anything during or after such freezes? -- tejun I'm very sorry, this was a false alarm / false assumption. Today, after many days without a freeze, my system just froze in the same manner as I described in the original mail to this list. This is _without_ libata. So it turns out that libata at most has an influence on the system, so that this problem occurs more frequently, but this is also not sure. I'll investigate this further and declare this thread officially closed - forget about it. ;) Patrick. -- Key ID: 0x86E346D4http://patrick-nagel.net/key.asc Fingerprint: 7745 E1BE FA8B FBAD 76AB 2BFC C981 E686 86E3 46D4 No PGP signature due to problems with webmailer plugin... (solving this one is on my todo list as well ;) ) - 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_platform: Fix NULL pointer dereference
Paul Mundt wrote: On Tue, Jul 17, 2007 at 05:33:00AM -0400, Jeff Garzik wrote: Paul Mundt wrote: It would be great if libata people could actually be bothered to CC driver authors on driver changes so that way these things don't get completely broken in mainline when simple testing on the platforms that actually _rely_ on this driver would have shown that this was broken. The patch lived for weeks in -mm along with tons of other git trees Andrew pulls. If you want a single point to watch for upcoming stuff, test the -mm trees. It's a key part of the development process: I merge a patch, -mm tree pulls my tree and others, people test and complain and give feedback, ... A key part of the development process is making sure that driver authors are aware of the changes being made to their drivers, so they're able to ACK/NACK or at least point out something that's obviously wrong. I test -mm when time permits, and this happened to slip through. If I had actually been CC'ed on it, it would not have. Your entire process is fundamentally flawed if you're merging the patch first and expecting people to only find out if it's broken after the fact. In the best case it leaves -mm broken for a single release, and in the other case, it happens to make its way to mainline before anyone notices. It's just reality that there are never enough people to verify every patch before it goes in. We just support way too much hardware for that to be remotely feasible. Sure we -try- to keep the driver maintainer CC'd, but alas we are human. Your thinking is flawed if you think I'm going to hold up every patch until it's verified on each of 63 ATA controller drivers, when I make a core change, for example. Not scalable. We scale in another way: We have the biggest test lab in the world -- the Internet -- and a development process staged so that testing and development occur in parallel. This here is actually an example of the process working: despite my forgetting to CC you, the problem was caught long before it made it into any release kernel. Jeff - 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] Fix SMART reporting on 2.6.22
On Mon, Jul 16, 2007 at 07:32:57PM +0900, Tejun Heo wrote: [cc'ing Jeff and Albert] Petr Vandrovec wrote: Fix reported task file values in sense data ata_tf_read was setting HOB bit when lba48 command was submitted, but was not clearing it before reading normal data. Maybe it would be better to just clear HOB bit immediately after reading upper halves for lba48 command, but I just decided to clear HOB bit in each ata_tf_read... Signed-off-by: Petr Vandrovec [EMAIL PROTECTED] Acked-by: Tejun Heo [EMAIL PROTECTED] Gee, thanks a lot for spotting this. This is definitely for -stable. Hmm... it's a separate issue and not your fault but ap-last_ctl caching is broken in the function. Albert is about to remove ap-last_ctl caching but we still need to fix it for -stable. Petr, are you interested in submitting a separate patch for fixing ap-last_ctl handling in the function? OK, that pushed me over edge so this replaces my previous patch. Now there is no ctl access when 24bit commands are issued back to back, and ctl is touched (twice...) only for 48bit commands. smartctl still works... Any other email address I should CC? Thanks, Petr Vandrovec Fix reported task file values in sense data ata_tf_read was setting HOB bit when lba48 command was submitted, but was not clearing it before reading normal data. As it is only place which sets HOB bit in control register, and register reads should not be affected by other bits, let's just clear it when we are done with reading upper bytes so non-48bit commands do not have to touch ctl at all. pata_scc suffered from same problem... Signed-off-by: Petr Vandrovec [EMAIL PROTECTED] --- commit d09591edad8e75c9bc850d47b68a9a6f6f107998 tree 1bd5a79c9cade9b7ebf68b13bf24a879b969e4c8 parent a5fcaa210626a79465321e344c91a6a7dc3881fa author Petr Vandrovec [EMAIL PROTECTED] Tue, 17 Jul 2007 03:45:43 -0700 committer Petr Vandrovec [EMAIL PROTECTED] Tue, 17 Jul 2007 03:45:43 -0700 drivers/ata/libata-sff.c |2 ++ drivers/ata/pata_scc.c |2 ++ 2 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index ca7d224..6a579a9 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -211,6 +211,8 @@ void ata_tf_read(struct ata_port *ap, struct ata_taskfile *tf) tf-hob_lbal = ioread8(ioaddr-lbal_addr); tf-hob_lbam = ioread8(ioaddr-lbam_addr); tf-hob_lbah = ioread8(ioaddr-lbah_addr); + iowrite8(tf-ctl, ioaddr-ctl_addr); + ap-last_ctl = tf-ctl; } } diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c index c55667e..60cce07 100644 --- a/drivers/ata/pata_scc.c +++ b/drivers/ata/pata_scc.c @@ -358,6 +358,8 @@ static void scc_tf_read (struct ata_port *ap, struct ata_taskfile *tf) tf-hob_lbal = in_be32(ioaddr-lbal_addr); tf-hob_lbam = in_be32(ioaddr-lbam_addr); tf-hob_lbah = in_be32(ioaddr-lbah_addr); + out_be32(ioaddr-ctl_addr, tf-ctl); + ap-last_ctl = tf-ctl; } } Fix reported task file values in sense data ata_tf_read was setting HOB bit when lba48 command was submitted, but was not clearing it before reading normal data. As it is only place which sets HOB bit in control register, and register reads should not be affected by other bits, let's just clear it when we are done with reading upper bytes so non-48bit commands do not have to touch ctl at all. pata_scc suffered from same problem... Signed-off-by: Petr Vandrovec [EMAIL PROTECTED] --- commit d09591edad8e75c9bc850d47b68a9a6f6f107998 tree 1bd5a79c9cade9b7ebf68b13bf24a879b969e4c8 parent a5fcaa210626a79465321e344c91a6a7dc3881fa author Petr Vandrovec [EMAIL PROTECTED] Tue, 17 Jul 2007 03:45:43 -0700 committer Petr Vandrovec [EMAIL PROTECTED] Tue, 17 Jul 2007 03:45:43 -0700 drivers/ata/libata-sff.c |2 ++ drivers/ata/pata_scc.c |2 ++ 2 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c index ca7d224..6a579a9 100644 --- a/drivers/ata/libata-sff.c +++ b/drivers/ata/libata-sff.c @@ -211,6 +211,8 @@ void ata_tf_read(struct ata_port *ap, struct ata_taskfile *tf) tf-hob_lbal = ioread8(ioaddr-lbal_addr); tf-hob_lbam = ioread8(ioaddr-lbam_addr); tf-hob_lbah = ioread8(ioaddr-lbah_addr); + iowrite8(tf-ctl, ioaddr-ctl_addr); + ap-last_ctl = tf-ctl; } } diff --git a/drivers/ata/pata_scc.c b/drivers/ata/pata_scc.c index c55667e..60cce07 100644 --- a/drivers/ata/pata_scc.c +++ b/drivers/ata/pata_scc.c @@ -358,6 +358,8 @@ static void scc_tf_read (struct ata_port *ap, struct ata_taskfile *tf) tf-hob_lbal = in_be32(ioaddr-lbal_addr); tf-hob_lbam = in_be32(ioaddr-lbam_addr);
Re: [PATCH] pata_platform: Fix NULL pointer dereference
On Tue, Jul 17, 2007 at 06:24:53AM -0400, Jeff Garzik wrote: It's just reality that there are never enough people to verify every patch before it goes in. We just support way too much hardware for that to be remotely feasible. Sure we -try- to keep the driver maintainer CC'd, but alas we are human. Try? I've never _once_ received a CC from you regarding a change to pata_platform. The only time I've been CC'ed at all is when someone submitting the patch had the common courtesy to do so. Whereas the number of times I've had to clean up after something merged behind my back has been higher than the number of times I've been CC'ed on any of these changes. Your thinking is flawed if you think I'm going to hold up every patch until it's verified on each of 63 ATA controller drivers, when I make a core change, for example. Not scalable. We scale in another way: This has nothing to do with core changes that impact every driver, this has to do with isolated changes to a _single_ driver that you're obviously not in a position to test. That's a very different situation. Again, if you can't be bothered to CC people on changes to their drivers that aren't libata-wide, don't apply them in the first place. I would much rather play API catch up with my driver than have to backtrack and figure out what went wrong when suddenly all of my boards stop booting. Your merge first and hope someone else notices and fixes it later approach is insane. We've never done kernel development like that, libata isn't special. - 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: fix last_ctl caching in ata_tf_read()
libata: fix last_ctl caching in ata_tf_read()
last_ctl was not cached properly. (Pointed out
by Tejun Heo.)
Signed-off-by: Chuck Ebbert [EMAIL PROTECTED]
---
(Apply after Petr's patch to fix SMART bugs.)
drivers/ata/libata-sff.c |4
1 file changed, 4 insertions(+)
--- 2.6.22-d390.orig/drivers/ata/libata-sff.c
+++ 2.6.22-d390/drivers/ata/libata-sff.c
@@ -196,7 +196,9 @@ void ata_tf_read(struct ata_port *ap, st
{
struct ata_ioports *ioaddr = ap-ioaddr;
+ ap-last_ctl = tf-ctl;
iowrite8(tf-ctl, ioaddr-ctl_addr);
+
tf-command = ata_check_status(ap);
tf-feature = ioread8(ioaddr-error_addr);
tf-nsect = ioread8(ioaddr-nsect_addr);
@@ -206,7 +208,9 @@ void ata_tf_read(struct ata_port *ap, st
tf-device = ioread8(ioaddr-device_addr);
if (tf-flags ATA_TFLAG_LBA48) {
+ ap-last_ctl = tf-ctl | ATA_HOB;
iowrite8(tf-ctl | ATA_HOB, ioaddr-ctl_addr);
+
tf-hob_feature = ioread8(ioaddr-error_addr);
tf-hob_nsect = ioread8(ioaddr-nsect_addr);
tf-hob_lbal = ioread8(ioaddr-lbal_addr);
-
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 1/3] Make the IDE DMA timeout modifiable
Hello. Rogier Wolff wrote: Ah, that makes sense -- during PIO interrupts happen a lot more often. 20 secs still seem to be too much. I don't think so, even for modern drives. Figure 8-10 seconds max for spin-up, plus 6-9 seconds to do a sector re-assignment or retries on a bad block (a measured *real-life* value). That adds up to 14-19 seconds, so 20 seconds is probably good. Still, this does need to be adjustable for faster (CF) devices, and slower (optical/tape) devices, rather than just a single set of fixed timeout values. In real life, with real bad blocks on real harddrives, some harddrives take more than the DMA TIMEOUT time to read a single block, even without having to spin up. Yeah, shit happens. The current code then resets the drive, on which the drive reports busy, not ready for command, and things go downhill from there. You are clearly mixing things: this message is a result of retrying command in PIO mode (that fails because the drive is still busy), and then the drives are actually reset. If they keep being busy *after* that, all I'd say is dump them ASAP. ;-) Roger. MBR, Sergei - 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 [ata_piix] still no resume from S3 ?
Hi to all! I'm on Debian Sid with kernel 2.6.21 and I still can't resume my laptop (Sony Vaio SZ2) from S3. Searched on the archive and it seems that SATA drives are getting problems when used via the new libata. When the laptop tries to resume, it can't access the hard drive anymore (the LED never blinks), the filesystem is mounted read-only and of course, the system hangs. Is this still a know problem? Does 2.6.22 solve this already? If you need more information about my environment, please ask :) Here's more info about the problem. I had to manually copy the trace, so I remove the hexadecimal parts (if you need them, please ask i Will try to copy them too). For now I just want to know if you can diagnose this problem, or need more info. So here's the interesting (I think) part of the log after resume: irq 23: nobody cared (try booting with the irqpool option) __report_bad_irq note_interrupt handle_IRQ_event handle_fasteoi_irq do_IRQ do_IRQ irq_exit smp_acpi_timer common_interrupt acpi_pm_read getnstimeofday ktime_get_ts ktime_egt tick_nohz_restart_sched_tick cpu_idle start_kernel unkown_bootoption = handlers: (ata_interrupt [libata]) (tifm_7xx1_isr [tifm_7xx1]) Disabling IRQ #23 (...) ATA: abnormal status 0x7F on port 0x000118cf And hard drive never cames up.. have to hard reboot the machine.. Any help? TIA Ruben -- Will work for bandwidth - 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 1/2] PCI: rename and export __pci_reenable_device()
On Wed, Jul 11, 2007 at 07:32:04PM +0900, Tejun Heo wrote: Some odd ACPI implementations choke if certain controller is disabled when ACPI suspend is invoked but we still need to make sure the PCI device is enabled during resume. Simply using pci_enable_device() unbalances device enable count. Drop leading underscores from __pci_reenable_device() and export it. Signed-off-by: Tejun Heo [EMAIL PROTECTED] Jeff, I'm guessing this will go through your tree as the ata patch after this needs this change. If so, feel free to add my: Signed-off-by: Greg Kroah-Hartman [EMAIL PROTECTED] to the patch, I have no objection to it. thanks, greg k-h - 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.c: fix CONFIG_PM=n compilation
Commit df69c9c5438b4e396a64d42608b2a6c48a3e7475 moved only prototype of
out of CONFIG_PM. Move function out as well. Box seems to boot fine.
Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED]
---
drivers/ata/ahci.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1519,6 +1519,14 @@ static void ahci_post_internal_cmd(struct ata_queued_cmd
*qc)
}
}
+static int ahci_port_resume(struct ata_port *ap)
+{
+ ahci_power_up(ap);
+ ahci_start_port(ap);
+
+ return 0;
+}
+
#ifdef CONFIG_PM
static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
{
@@ -1536,14 +1544,6 @@ static int ahci_port_suspend(struct ata_port *ap,
pm_message_t mesg)
return rc;
}
-static int ahci_port_resume(struct ata_port *ap)
-{
- ahci_power_up(ap);
- ahci_start_port(ap);
-
- return 0;
-}
-
static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
{
struct ata_host *host = dev_get_drvdata(pdev-dev);
-
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: block/bsg.c
Hi,
Some more bsg findings...
block/Kconfig:
endif # BLOCK
Shouldn't BLK_DEV_BSG depend on BLOCK?
config BLK_DEV_BSG
bool Block layer SG support
depends on (SCSI=y) EXPERIMENTAL
default y
---help---
Saying Y here will enable generic SG (SCSI generic) v4
support for any block device.
block/bsg.c:
...
/*
* TODO
* - Should this get merged, block/scsi_ioctl.c will be migrated into
*this file. To keep maintenance down, it's easier to have them
*seperated right now.
*
*/
This TODO should be fixed/removed.
Firstly bsg got merged. ;) Moreover bsg depends on SCSI and scsi_ioctl doesn't.
Even if SCSI dependency is fixed bsg requires block driver to have struct
class devices which is not a case for scsi_ioctl.
...
static struct bsg_device *__bsg_get_device(int minor)
{
struct hlist_head *list = bsg_device_list[bsg_list_idx(minor)];
bsg_device_list[] access should be done under bsg_mutex lock.
May not be a problem currently because of lock_kernel but worth fixing anyway.
struct bsg_device *bd = NULL;
struct hlist_node *entry;
mutex_lock(bsg_mutex);
...
static int
bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
unsigned long arg)
{
...
case SCSI_IOCTL_SEND_COMMAND: {
Do we really want to add support for this *deprecated* ioctl to
the *new* and shiny bsg driver?
void __user *uarg = (void __user *) arg;
return scsi_cmd_ioctl(file, bd-queue, NULL, cmd, uarg);
}
...
int bsg_register_queue(struct request_queue *q, const char *name)
{
...
memset(bcd, 0, sizeof(*bcd));
...
dev = MKDEV(BSG_MAJOR, bcd-minor);
class_dev = class_device_create(bsg_class, NULL, dev, bcd-dev, %s,
name);
bcd-dev is always 0 (NULL).
Is it OK to pass NULL struct device *dev argument to class_device_create()?
It should be fixed by either removing bcd-dev or by setting it to something
other than zero.
...
MODULE_AUTHOR(Jens Axboe);
MODULE_DESCRIPTION(Block layer SGSI generic (sg) driver);
SGSI? :)
MODULE_LICENSE(GPL);
Now back to the first bsg commit 3d6392cfbd7dc11f23058e3493683afab4ac13a3:
diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index f429be8..a21f585 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -1258,19 +1258,25 @@ static void idefloppy_create_rw_cmd (idefloppy_floppy_t
*floppy, idefloppy_pc_t
set_bit(PC_DMA_RECOMMENDED, pc-flags);
}
-static int
+static void
idefloppy_blockpc_cmd(idefloppy_floppy_t *floppy, idefloppy_pc_t *pc, struct
request *rq)
{
- /*
-* just support eject for now, it would not be hard to make the
-* REQ_BLOCK_PC support fully-featured
-*/
- if (rq-cmd[0] != IDEFLOPPY_START_STOP_CMD)
- return 1;
-
idefloppy_init_pc(pc);
+ pc-callback = idefloppy_rw_callback;
memcpy(pc-c, rq-cmd, sizeof(pc-c));
- return 0;
+ pc-rq = rq;
+ pc-b_count = rq-data_len;
+ if (rq-data_len rq_data_dir(rq) == WRITE)
+ set_bit(PC_WRITING, pc-flags);
+ pc-buffer = rq-data;
+ if (rq-bio)
+ set_bit(PC_DMA_RECOMMENDED, pc-flags);
Great to see SG_IO support being added to ide-floppy but this change has
nothing to do with the addition of bsg driver so WTF they have been mixed
within the same commit?
I also can't recall seeing this patch on linux-ide mailing list...
+ /*
+* possibly problematic, doesn't look like ide-floppy correctly
+* handled scattered requests if dma fails...
+*/
Could you give some details on this?
+ pc-request_transfer = pc-buffer_size = rq-data_len;
}
/*
@@ -1317,10 +1323,7 @@ static ide_startstop_t idefloppy_do_request (ide_drive_t
*drive, struct request
pc = (idefloppy_pc_t *) rq-buffer;
} else if (blk_pc_request(rq)) {
pc = idefloppy_next_pc_storage(drive);
- if (idefloppy_blockpc_cmd(floppy, pc, rq)) {
- idefloppy_do_end_request(drive, 0, 0);
- return ide_stopped;
- }
+ idefloppy_blockpc_cmd(floppy, pc, rq);
} else {
blk_dump_rq_flags(rq,
ide-floppy: unsupported command in queue);
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index c948a5c..9ae60a7 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
ide.c changes also have nothing to do with addition of bsg driver.
@@ -1049,9 +1049,13 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file
*file, struct block_device
unsigned long flags;
ide_driver_t *drv;
void __user *p = (void __user *)arg;
- int err = 0, (*setfunc)(ide_drive_t *, int);
+ int err, (*setfunc)(ide_drive_t *, int);
u8 *val;
+ err = scsi_cmd_ioctl(file, bdev-bd_disk, cmd, p);
+ if (err != -ENOTTY)
+
Re: block/bsg.c
On Tue, 17 Jul 2007 22:52:25 +0200
Bartlomiej Zolnierkiewicz [EMAIL PROTECTED] wrote:
ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
(so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
will result in requests being failed and error messages in the kernel logs).
In my case it results in a completely tits-up machine. CPU stuck somewhere
spinning with local interrutps disabled.
If you see the log output I sent, I'm suspecting this might be because
this BSG brainfart has exposed underlying bugs in IDE or SCSI.
Note that I saw literally 20,000 lines of output in the bit which I snipped,
so something has gone pretty wild in the handling of these bogus commands.
-
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
[git patches 1/2] warnings: attack valid cases spotted by warnings
Those may be used uninitialized warnings are annoying. So annoying that kernel developers tune them out, and that occasionally hides real bugs, as my past patches (and those below) indicate. I included the full-length changelog below the diffstat, because that is where the best explanation for each change is found. In most cases that's where all the length is -- a paragraph or two describing what is usually a one-line code change, usually an initialization or simple cleanup. This is the first of two git pull sets, the -parent-. If you pull 'warnings' branch, you get what you see below and nothing else. WYSIWYG. You'll see in the second email why I distinguish the two. Please pull from 'warnings' branch of master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings Jeff Garzik (11): kernel/auditfilter: kill bogus uninit'd-var compiler warning [netdrvr] natsemi: Fix device removal bug [netdrvr] eepro100, ne2k-pci: abort resume if pci_enable_device() fails drivers/usb/misc/auerswald: fix status check, remove redundant check drivers/net/wan/pc300_drv: fix bug caught by gcc warning drivers/telephony/ixj: cleanup and fix gcc warning drivers/mtd/ubi/eba: minor cleanup: tighten scope of a local var drivers/net/wan/sbni: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning [libata] sata_mv: use pci_try_set_mwi() drivers/atm/ambassador: kill uninit'd var warning, and fix bug drivers/ata/sata_mv.c |2 +- drivers/atm/ambassador.c |4 +++- drivers/infiniband/hw/mthca/mthca_qp.c |4 ++-- drivers/mtd/ubi/eba.c |4 ++-- drivers/net/eepro100.c |7 ++- drivers/net/natsemi.c |2 +- drivers/net/ne2k-pci.c |7 ++- drivers/net/wan/pc300_drv.c|2 ++ drivers/net/wan/sbni.c |7 +++ drivers/telephony/ixj.c|7 ++- drivers/usb/misc/auerswald.c |2 +- kernel/auditfilter.c | 12 12 files changed, 37 insertions(+), 23 deletions(-) commit b1734d2388cc45ecdec58615e35955d0d402f938 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 02:32:21 2007 -0400 drivers/atm/ambassador: kill uninit'd var warning, and fix bug An uninitialized variable warning illuminated an area where indeed the variable was being used without initialization. Unfortunately, after verifying all such paths were fixed, the warning still appears. So we follow the initialization practice of other variables in this function. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit ea8b4db97aa41a66c05daa4055a1974692ccd52d Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 02:21:50 2007 -0400 [libata] sata_mv: use pci_try_set_mwi() Because sometimes in life, it's ok to fail. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit 9db48926208562df3c778682e064990170ab8971 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 02:03:49 2007 -0400 drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_tavor_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ‘f0’ may be used uninitialized in this function drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_arbel_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: ‘f0’ may be used uninitialized in this function Initializing 'f0' is not strictly necessary in either case, AFAICS. I was considering use of uninitialized_var(), but looking at the complex flow of control in each function, I feel it is wiser and safer to simply zero the var and be certain of ourselves. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit e5fb4f42268654ca41ab50b1406fb7da97559db5 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:56:32 2007 -0400 drivers/net/wan/sbni: kill uninit'd var warning It's actually convenient in the code to initialize this and a sister variable to zero. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit 2ab934b8afa89b9b3e71b7fb66470a19772f5012 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:49:56 2007 -0400 drivers/mtd/ubi/eba: minor cleanup: tighten scope of a local var Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit 0d480db85dea59e1393c3968fbdac0117431e797 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:35:08 2007 -0400 drivers/telephony/ixj: cleanup and fix gcc warning 1) Fix gcc uninit'd var warnings by adding 'default' switch stmt labels in two cases. It was lightning-strikes unlikely that a problem would ever arise, but not impossible. 2) Tighten the scope of 'blankword' in two cases.
[git patches 2/2] warnings: use uninitialized_var()
For many months, I have maintained a hand-verified list of bogus may be
used uninitialized warning fixes, in misc-2.6.git#gccbug. Andrew urged
me to head these upstream.
I have gone through and re-analyzed each warning, and verified that
these variables are indeed initialized properly, and gcc is making
needless noise.
These uninitialized_var() markers are in a separate branch from the
other warning fixes/silences to enable people to object to this
separately.
Potential upsides of pulling 'uninit-var':
* reduces noise proven to hide bugs
* pushes upstream the work I have done, hand-verifying that each
warning so marked is indeed bogus.
* uninitialized_var a grep-friendly symbol
* uninitialized_var could be disabled via Kconfig, if someone
really wanted to see the warnings again
Potential downsides to uninitialized_var():
* Future code changes related to a marked variable could potentially
result in hiding a valid uninit'd-var warning (i.e. hiding a bug).
* uninitialized_var stands out in the code. Ugly? Or a good thing?
This is the second of two git pulls, the -child-. If you pull branch
'uninit-var', you will receive BOTH 'uninit-var' and 'warnings'
branches.
Please pull from 'uninit-var' branch of
master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git uninit-var
commit 8e1c091cccd551557d24ce845715e8ceb6c49d36
Author: Jeff Garzik [EMAIL PROTECTED]
Date: Tue Jul 17 05:40:59 2007 -0400
arch/i386/* fs/* ipc/*: mark variables with uninitialized_var()
Mark variables with uninitialized_var() if such a warning appears,
and analysis proves that the var is initialized properly on all paths
it is used.
Signed-off-by: Jeff Garzik [EMAIL PROTECTED]
commit a6343afb6e16b65b9f0b264f94f8207212e7e3ae
Author: Jeff Garzik [EMAIL PROTECTED]
Date: Tue Jul 17 05:39:58 2007 -0400
drivers/*: mark variables with uninitialized_var()
Mark variables in drivers/* with uninitialized_var() if such a warning
appears, and analysis proves that the var is initialized properly on all
paths it is used.
Signed-off-by: Jeff Garzik [EMAIL PROTECTED]
arch/i386/kernel/efi.c|2 +-
drivers/atm/zatm.c|4 +++-
drivers/char/cyclades.c |4 ++--
drivers/mtd/ubi/eba.c |2 +-
drivers/net/r8169.c |2 +-
drivers/net/tokenring/smctr.c |6 --
drivers/usb/misc/auerswald.c |2 +-
drivers/video/matrox/matroxfb_maven.c |9 +++--
drivers/video/riva/riva_hw.c |7 ++-
fs/ocfs2/file.c |3 ++-
fs/udf/super.c|2 +-
ipc/msg.c |4 ++--
ipc/sem.c |2 +-
13 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/arch/i386/kernel/efi.c b/arch/i386/kernel/efi.c
index a180802..2452c6f 100644
--- a/arch/i386/kernel/efi.c
+++ b/arch/i386/kernel/efi.c
@@ -278,7 +278,7 @@ void efi_memmap_walk(efi_freemem_callback_t callback, void
*arg)
struct range {
unsigned long start;
unsigned long end;
- } prev, curr;
+ } uninitialized_var(prev), curr;
efi_memory_desc_t *md;
unsigned long start, end;
void *p;
diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
index 020a87a..58583c6 100644
--- a/drivers/atm/zatm.c
+++ b/drivers/atm/zatm.c
@@ -915,7 +915,7 @@ static int open_tx_first(struct atm_vcc *vcc)
unsigned long flags;
u32 *loop;
unsigned short chan;
- int pcr,unlimited;
+ int unlimited;
DPRINTK(open_tx_first\n);
zatm_dev = ZATM_DEV(vcc-dev);
@@ -936,6 +936,8 @@ static int open_tx_first(struct atm_vcc *vcc)
vcc-qos.txtp.max_pcr = ATM_OC3_PCR);
if (unlimited zatm_dev-ubr != -1) zatm_vcc-shaper = zatm_dev-ubr;
else {
+ int uninitialized_var(pcr);
+
if (unlimited) vcc-qos.txtp.max_sdu = ATM_MAX_AAL5_PDU;
if ((zatm_vcc-shaper = alloc_shaper(vcc-dev,pcr,
vcc-qos.txtp.min_pcr,vcc-qos.txtp.max_pcr,unlimited))
diff --git a/drivers/char/cyclades.c b/drivers/char/cyclades.c
index 7b08394..9e0adfe 100644
--- a/drivers/char/cyclades.c
+++ b/drivers/char/cyclades.c
@@ -4466,10 +4466,10 @@ static void cy_hangup(struct tty_struct *tty)
static int __devinit cy_init_card(struct cyclades_card *cinfo)
{
struct cyclades_port *info;
- u32 mailbox;
+ u32 uninitialized_var(mailbox);
unsigned int nports;
unsigned short chip_number;
- int index, port;
+ int uninitialized_var(index), port;
spin_lock_init(cinfo-card_lock);
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 4dc10c8..7c6b223 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -368,7 +368,7 @@ int ubi_eba_read_leb(struct ubi_device *ubi, int vol_id,
int lnum,
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function âmthca_tavor_post_sendâ: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: âf0â may be used uninitialized in this function drivers/infiniband/hw/mthca/mthca_qp.c: In function âmthca_arbel_post_sendâ: drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: âf0â may be used uninitialized in this function Initializing 'f0' is not strictly necessary in either case, AFAICS. I was considering use of uninitialized_var(), but looking at the complex flow of control in each function, I feel it is wiser and safer to simply zero the var and be certain of ourselves. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] I don't really like this. These functions are in the hottest, most latency-sensitive code path of this driver, which is used by people who care about nanoseconds. I'm quite confident that the code is correct as written, and it really feels wrong to me to add bloat to the fastpath just to cover up a shortcoming of gcc. And I don't like using uninitialized_var() here for a similar reason... the functions are complex and I would prefer to avoid hiding future bugs that may creep in. In fact adding the initialization to 0 has a similar effect, since it shuts up the compiler even if the logic in the function gets screwed up. On the other hand I definitely sympathize with the desire to have a warning-free build so that real warnings are more visible. I guess I could live with the uninitialized_var() patch, although I would feel a little sad. - R. - 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
Roland Dreier wrote: drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_tavor_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ‘f0’ may be used uninitialized in this function drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_arbel_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: ‘f0’ may be used uninitialized in this function Initializing 'f0' is not strictly necessary in either case, AFAICS. I was considering use of uninitialized_var(), but looking at the complex flow of control in each function, I feel it is wiser and safer to simply zero the var and be certain of ourselves. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] I don't really like this. These functions are in the hottest, most latency-sensitive code path of this driver, which is used by people who care about nanoseconds. I'm quite confident that the code is correct as written, and it really feels wrong to me to add bloat to the fastpath just to cover up a shortcoming of gcc. I don't buy that performance argument, in this case. You are already dirtying the same cacheline with other variable initializations. Like I noted in the changeset description (hey, this is precisely why I included it, so that we could have this discussion), IMO the flow of control makes it not only impossible for the compiler to understand the full value range of 'f0', but also difficult for humans as well. I could perhaps understand initializing the variable to some poison value rather than zero, but IMO the code is stronger with f0 set to a sane value. It's poorly readable, poorly commented code as-is. Jeff - 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
Jeff Garzik wrote: I don't buy that performance argument, in this case. You are already dirtying the same cacheline with other variable initializations. Or simply sitting in a CPU register for large stretches of function runtime... Jeff - 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007 14:53:02 -0700 Roland Dreier [EMAIL PROTECTED] wrote: drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function __mthca_tavor_post_send__: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: __f0__ may be used uninitialized in this function (rofl, look at that mess: it was utterly impractical, unrealistic and stupid for gcc to go and UTFify the compiler output. Sigh. LANG=C, guys). And I don't like using uninitialized_var() here for a similar reason... the functions are complex and I would prefer to avoid hiding future bugs that may creep in. umm, the code *already* produces a warning. So if you later add a real used-uninitialised bug, you won't know about it, because everyone was trained to ignore the warning anyway. Best would be to find some way to restructure the code to make the warning go away. Meanwhile I'd say switch this to uninitialized_var() if it's that much of a worry. - 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Andrew Morton wrote:
(rofl, look at that mess: it was utterly impractical, unrealistic and
stupid for gcc to go and UTFify the compiler output. Sigh. LANG=C, guys).
Yeah, I absolutely *detest* how gcc does idiotic quoting just because you
happen to be in a UTF-8 locale. There's no reason for it what-so-ever, and
I don't understand the mindset of whoever did that crap.
They should use a nice ASCII tick-mark ('), not some fancy quotation
marks.
Linus
-
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: block/bsg.c
From: Bartlomiej Zolnierkiewicz [EMAIL PROTECTED]
Subject: Re: block/bsg.c
Date: Tue, 17 Jul 2007 22:52:25 +0200
/*
* TODO
* - Should this get merged, block/scsi_ioctl.c will be migrated into
*this file. To keep maintenance down, it's easier to have them
*seperated right now.
*
*/
This TODO should be fixed/removed.
Yeah, this should be removed now. I'll do.
Firstly bsg got merged. ;) Moreover bsg depends on SCSI and scsi_ioctl
doesn't.
Even if SCSI dependency is fixed bsg requires block driver to have struct
class devices which is not a case for scsi_ioctl.
...
static struct bsg_device *__bsg_get_device(int minor)
{
struct hlist_head *list = bsg_device_list[bsg_list_idx(minor)];
bsg_device_list[] access should be done under bsg_mutex lock.
May not be a problem currently because of lock_kernel but worth fixing anyway.
struct bsg_device *bd = NULL;
struct hlist_node *entry;
mutex_lock(bsg_mutex);
They were fixed. Please check the latest code:
git://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-2.6-block.git bsg
We wait for Linus to pull from it.
static int
bsg_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
unsigned long arg)
{
...
case SCSI_IOCTL_SEND_COMMAND: {
Do we really want to add support for this *deprecated* ioctl to
the *new* and shiny bsg driver?
void __user *uarg = (void __user *) arg;
return scsi_cmd_ioctl(file, bd-queue, NULL, cmd, uarg);
}
We might remove it.
int bsg_register_queue(struct request_queue *q, const char *name)
{
...
memset(bcd, 0, sizeof(*bcd));
...
dev = MKDEV(BSG_MAJOR, bcd-minor);
class_dev = class_device_create(bsg_class, NULL, dev, bcd-dev, %s,
name);
bcd-dev is always 0 (NULL).
Is it OK to pass NULL struct device *dev argument to class_device_create()?
It's ok, I guess.
It should be fixed by either removing bcd-dev or by setting it to something
other than zero.
...
MODULE_AUTHOR(Jens Axboe);
MODULE_DESCRIPTION(Block layer SGSI generic (sg) driver);
SGSI? :)
It was fixed in the latest code.
Thanks,
-
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: block/bsg.c
On Tuesday 17 July 2007, Andrew Morton wrote:
On Tue, 17 Jul 2007 22:52:25 +0200
Bartlomiej Zolnierkiewicz [EMAIL PROTECTED] wrote:
ide-{disk,scsi,tape} don't support REQ_TYPE_BLOCK_PC requests et all
(so attempts to use SG_IO, CDROM_SEND_PACKET and SCSI_IOCTL_SEND_COMMAND
will result in requests being failed and error messages in the kernel logs).
In my case it results in a completely tits-up machine. CPU stuck somewhere
spinning with local interrutps disabled.
If you see the log output I sent, I'm suspecting this might be because
I don't. :(
this BSG brainfart has exposed underlying bugs in IDE or SCSI.
Possible but ATM I have an (overdue) IDE update to push, people waiting for
(overdue) patches to try and some (overdue) patches to finish and post...
Would be great if somebody beats me to investigate this issue.
Hint: this would be a great way to redempt pushing buggy commit behind
my back. ;-)
Thanks,
Bart
-
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: Recommendations on user-space soft/hardreset
- each driver, in ata_port_operations, specifies a soft_reset and hard_reset functions, which take ata_port* as a parameter They already have them via the EH code. - in ata_scsi_pass_through, check for HARDRESET and SRST, and call respective driver's soft/hard reset function Old IDE has an ioctl for it - it would be good to provide this for compatibility as we have with some other needed ioctls. Now here's where I'm not quite certain on how to proceed. Another developer has recommended that the reset be performed from an error context, so that pre/post reset get called automatically. However, there's a few issues, one of which is, as I was told, that the user process will resume before the reset is complete. Thus the user sends SG_IO and gets an immediate response, before the reset is finished, whereas running the reset command directly blocks the user process until the drive returns. So the question is: should the driver simply call prereset, optionally hardreset, softreset, and postreset, or set up the error handler context to do it? I think you have to do the latter. I see no other way to get the locking right, and the old IDE was horrible (and broken) because it tried to do it in the user context. drivers/ide is a worked example of why *NOT* to simply call the methods. The EH just needs triggering and can do the rest sanely in a sensible context without adding new methods to the drivers. - 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: Recommendations on user-space soft/hardreset
As an example, I'll use the sata_sil24 driver, as that's what I'll be
using on my machine (until sata_mv comes around)
On 7/17/07, Alan Cox [EMAIL PROTECTED] wrote:
- each driver, in ata_port_operations, specifies a soft_reset and
hard_reset functions, which take ata_port* as a parameter
They already have them via the EH code.
You mean:
static const struct ata_port_operations sil24_ops = {
...
.error_handler = sil24_error_handler,
...
which then calls
ata_do_eh(ap, ata_std_prereset, sil24_softreset, sil24_hardreset,
ata_std_postreset);
How to then specify if you just need a softreset? My understanding is
all those functions get called.
- in ata_scsi_pass_through, check for HARDRESET and SRST, and call
respective driver's soft/hard reset function
Old IDE has an ioctl for it - it would be good to provide this for
compatibility as we have with some other needed ioctls.
Well, the SAT way of specifying soft/hardresets I found at [1] on page
104, and the basis for this is already in libata, just not
implemented: see ata_scsi_map_proto. Implementing it there seems more
compatible with the standard.
So the question is: should the driver simply call prereset, optionally
hardreset, softreset, and postreset, or set up the error handler
context to do it?
I think you have to do the latter. I see no other way to get the locking
right, and the old IDE was horrible (and broken) because it tried to do
it in the user context. drivers/ide is a worked example of why *NOT* to
simply call the methods. The EH just needs triggering and can do the rest
sanely in a sensible context without adding new methods to the drivers.
Ugh.. I figured, I was just hoping I wouldn't have to deal with that.
Any idea on what happens to the user process (blocking, etc.) during
error handling? Surely if a disk needs to be hardreset during a normal
error, the user process doesn't hang for up to 30 seconds?
- Yasha
-
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: Recommendations on user-space soft/hardreset
Sorry forgot to link to http://www.t10.org/drafts.htm , one of which is: http://www.t10.org/ftp/t10/drafts/sat/sat-r09.pdf Page 104 was what I was following for my initial implementation plan. - 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
I don't buy that performance argument, in this case. You are already dirtying the same cacheline with other variable initializations. Like I noted in the changeset description (hey, this is precisely why I included it, so that we could have this discussion), IMO the flow of control makes it not only impossible for the compiler to understand the full value range of 'f0', but also difficult for humans as well. I could perhaps understand initializing the variable to some poison value rather than zero, but IMO the code is stronger with f0 set to a sane value. The more I think about it, the less sense initializing f0 to 0 makes. The whole problem with an uninitialized variable is that a random value from the stack might be used. So setting a variable to something meaningless (guaranteeing that a garbage value is used in case of a bug) just to shut up a warning makes no sense -- it's no safer than leaving the code as is. uninitialized_var() gets rid of the warning, saves a little text and instruction cache, and documents things better. (BTW, I agree the code is a little confusing as written. I think things could be cleaned up and made more efficient by getting rid of the initialization of size0 too -- I'll look at doing that) Anyway, I queued this up for my next merge with Linus: commit 6d7d080e9f7cd535a8821efd3835c5cfa5223ab6 Author: Roland Dreier [EMAIL PROTECTED] Date: Tue Jul 17 19:30:51 2007 -0700 IB/mthca: Use uninitialized_var() for f0 Commit 9db48926 (drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning) added = 0 to the declarations of f0 to shut up gcc warnings. However, there's no point in making the code bigger by initializing f0 to a random value just to get rid of a warning; setting f0 to 0 is no safer than just using uninitialized_var(), which documents the situation better and gives smaller code too. For example, on x86_64: add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-16 (-16) function old new delta mthca_tavor_post_send 13521344 -8 mthca_arbel_post_send 14891481 -8 Signed-off-by: Roland Dreier [EMAIL PROTECTED] diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c index 11f1d99..0e9ef24 100644 --- a/drivers/infiniband/hw/mthca/mthca_qp.c +++ b/drivers/infiniband/hw/mthca/mthca_qp.c @@ -1591,7 +1591,13 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, int i; int size; int size0 = 0; - u32 f0 = 0; + /* +* f0 is only used if nreq != 0, and f0 will be initialized +* the first time through the main loop, since size0 == 0 the +* first time through. So nreq cannot become non-zero without +* initializing f0, and f0 is in fact never used uninitialized. +*/ + u32 uninitialized_var(f0); int ind; u8 op0 = 0; @@ -1946,7 +1952,13 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, int i; int size; int size0 = 0; - u32 f0 = 0; + /* +* f0 is only used if nreq != 0, and f0 will be initialized +* the first time through the main loop, since size0 == 0 the +* first time through. So nreq cannot become non-zero without +* initializing f0, and f0 is in fact never used uninitialized. +*/ + u32 uninitialized_var(f0); int ind; u8 op0 = 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Roland Dreier wrote: So setting a variable to something meaningless (guaranteeing that a garbage value is used in case of a bug) just to shut up a warning makes no sense -- it's no safer than leaving the code as is. Wrong. It's safer for two reasons: - now everybody will see the *same* behaviour - the meaningless value is guaranteed to not be a security leak but the whole shut up bogus warnings is the best reason. So it *is* safer than leaving the code as-is. Of course, usually the best approach is to rewrite the code to be simpler, so that even gcc sees that something is obviously initialized. Sadly, people seldom do the right thing, and sometimes gcc just blows incredibly hard. Linus - 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] Fix SMART reporting on 2.6.22
Fix reported task file values in sense data ata_tf_read was setting HOB bit when lba48 command was submitted, but was not clearing it before reading normal data. As it is only place which sets HOB bit in control register, and register reads should not be affected by other bits, let's just clear it when we are done with reading upper bytes so non-48bit commands do not have to touch ctl at all. pata_scc suffered from same problem... Signed-off-by: Petr Vandrovec [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: [git patches 1/2] warnings: attack valid cases spotted by warnings
So setting a variable to something meaningless (guaranteeing that a
garbage value is used in case of a bug) just to shut up a warning makes
no sense -- it's no safer than leaving the code as is.
Wrong.
It's safer for two reasons:
- now everybody will see the *same* behaviour
- the meaningless value is guaranteed to not be a security leak
but the whole shut up bogus warnings is the best reason.
So it *is* safer than leaving the code as-is.
OK, fair enough. What I said wasn't quite right, but in my case I
think neither of your reasons really applies, since the uninitialized
variable would be written into some hardware control block, so the
effect would probably still be random even if the value is the same
and the information leak doesn't really matter.
Anyway, I think that in this case it's not too hard to show that the
variable really can't be used uninitialized, so I prefer the smaller
generated code from uninitialized_var() (plus a comment explaining why
that's safe).
Of course, usually the best approach is to rewrite the code to be simpler,
so that even gcc sees that something is obviously initialized. Sadly,
people seldom do the right thing, and sometimes gcc just blows incredibly
hard.
In this case the code is basically
u32 x;
for (n = 0; cond; ++n) {
...
if (!n)
x = something;
...
}
if (n) {
...
use(x);
...
}
and gcc still warns...
- R.
-
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: [git patches 1/2] warnings: attack valid cases spotted by warnings
Roland Dreier wrote:
In this case the code is basically
u32 x;
for (n = 0; cond; ++n) {
...
if (!n)
x = something;
...
}
if (n) {
...
use(x);
...
}
and gcc still warns...
Interestingly, the above accurately describes a common code pattern
matching code which caused gcc to emit the uninit'd-var warnings.
For the record I think initializating 'f0' to zero is safer for the
reasons Linus gave, and in addition, f0 is or'd with a value written to
a hardware register, which means things should go awry (if they go) in a
semi-predictable manner.
According to the assembly language produced, sure it is larger -- by one
(per function) MOV that is adjacent to other initializations, making it
highly likely the initializations are all streamed together. I doubt
one MOV per function will make a huge difference, considering the peace
of mind it buys.
Jeff
-
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: Recommendations on user-space soft/hardreset
Yasha Okshtein wrote: I need to send soft and/or hard resets from userspace. Actually, you can do it by requesting manual rescan of the SCSI host. echo - - - /sys/class/scsi_host/hostX/scan. -- 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
CONFIG_IDE_PROC_FS: /sys is not full replacement of /proc
May be I miss something obvious but most information that was available in /proc/ide is missing under /sys. At the very least, Mandriva hardware detection expects /proc/ide/hdX/model; nothing close is under /sys. Are there any plans to extend IDE /sys interface to provide full range of information from /proc? Alternatively I appreciate description (or pointer to thereof) how to get information that was available under /proc/ide without /proc/ide :) Thank you -andrey signature.asc Description: This is a digitally signed message part.
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Roland Dreier wrote:
I think this patch (on top of the previous one) actually makes the
code clearer
Quite frankly, calling this making the code clearer is a bit ridiculous.
That code still is absolute *crap* from a readability angle. It doesn't
follow any sane coding standards, and certainly not the most important
ones (keep the function small, don't have tons of local variables),
and it has absolutely ridiculously ugly casts that get repeated over and
over and over again.
Quite frankly, I don't quite understand where you get those enormous balls
you have, that you can then talk about how ugly it is to just add a = 0
that shuts up a compiler warning. That's the _least_ ugly part of the
whole damn function!
So rather than sending out that idiotic patch, look at that code for five
seconds, and ponder whether it really needs to be that ugly.
Here's a few things that you could *really* do to make it somewhat more
readable:
- make that whole switch() statement from hell another function
entirely, and have it return the size of the thing, so that you don't
need to have
wqe += xxx
size += xxx / 16;
repeated fifty times (and so that it's also obvious that the
always matches).
- make each switch case actually call a small function with the argument
cast to the right pointer type, so that you need *one* cast per case,
rather than a handful.
End result? More readable source code, with functions that are 20 lines
long (or less), rather than 200 lines of spagetti-coding.
And you know what? That's actually more important than 16 bytes of object
code, although looking at the size of the infiniband code, I *seriously*
doubt any infiniband person has ever cared about object code size in their
life. That thing is not for weak machines or stomachs.
The warnign (and fixing it up) is the _least_ of the problems in that
code, methinks.
Anyway, here's a totally untested cleanup that compiles but probably
doesn't work, because I didn't check that I did the right thing with all
the pointer arithmetic (ie when I change wqe to a real structure pointer
instead of just a void *, maybe I left some pointer arithmetic around
that expected it to work as a byte pointer, but now really works on the
whole structure size instead).
So this patch is NOT meant to be applied, but it is meant to teach people
how things like this should be done. They should *not* be one big function
with lots of case statements. They should be lots of small functions!
Linus
---
drivers/infiniband/hw/mthca/mthca_qp.c | 236 ---
1 files changed, 122 insertions(+), 114 deletions(-)
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c
b/drivers/infiniband/hw/mthca/mthca_qp.c
index 11f1d99..74da9bc 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1578,6 +1578,113 @@ static inline int mthca_wq_overflow(struct mthca_wq
*wq, int nreq,
return cur + nreq = wq-max;
}
+static int handle_next_seg(struct ib_send_wr *wr, struct mthca_next_seg * wqe)
+{
+ wqe-nda_op = 0;
+ wqe-ee_nds = 0;
+ wqe-flags =((wr-send_flags IB_SEND_SIGNALED) ?
+cpu_to_be32(MTHCA_NEXT_CQ_UPDATE) : 0) |
+ ((wr-send_flags IB_SEND_SOLICITED) ?
+cpu_to_be32(MTHCA_NEXT_SOLICIT) : 0) |
+ cpu_to_be32(1);
+
+ if (wr-opcode == IB_WR_SEND_WITH_IMM ||
+ wr-opcode == IB_WR_RDMA_WRITE_WITH_IMM)
+ wqe-imm = wr-imm_data;
+
+ return sizeof(struct mthca_next_seg);
+}
+
+static int handle_raddr_seg(struct mthca_dev *dev, struct mthca_qp *qp, struct
ib_send_wr *wr,
+ struct mthca_raddr_seg *wqe, int ind)
+{
+ switch (qp-transport) {
+ case RC:
+ switch (wr-opcode) {
+ case IB_WR_ATOMIC_CMP_AND_SWP:
+ case IB_WR_ATOMIC_FETCH_AND_ADD: {
+ struct mthca_atomic_seg *atomic;
+
+ wqe-raddr = cpu_to_be64(wr-wr.atomic.remote_addr);
+ wqe-rkey = cpu_to_be32(wr-wr.atomic.rkey);
+ wqe-reserved = 0;
+
+ atomic = (struct mthca_atomic_seg *) (wqe+1);
+
+ if (wr-opcode == IB_WR_ATOMIC_CMP_AND_SWP) {
+ atomic-swap_add =
cpu_to_be64(wr-wr.atomic.swap);
+ atomic-compare =
cpu_to_be64(wr-wr.atomic.compare_add);
+ } else {
+ atomic-swap_add =
cpu_to_be64(wr-wr.atomic.compare_add);
+ atomic-compare = 0;
+ }
+
+ return sizeof (struct mthca_raddr_seg) +
+ sizeof (struct mthca_atomic_seg);
+ }
+
+ case IB_WR_RDMA_WRITE:
+ case
Re: Recommendations on user-space soft/hardreset
Well ideally this should work with a port multiplier to soft/hardreset only one disk behind the multiplier. Also, this again doesn't allow the user to specify soft or hard reset - I believe rescanning forces hardresets. Fully implementing the SATA spec, which should eventually be done anyway, seems like a cleaner solution. On 7/17/07, Tejun Heo [EMAIL PROTECTED] wrote: Yasha Okshtein wrote: I need to send soft and/or hard resets from userspace. Actually, you can do it by requesting manual rescan of the SCSI host. echo - - - /sys/class/scsi_host/hostX/scan. -- tejun -- I have never let my schooling interfere with my education. - Samuel Langhorne Clemens - 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: Recommendations on user-space soft/hardreset
Yasha Okshtein wrote: Well ideally this should work with a port multiplier to soft/hardreset only one disk behind the multiplier. You can do that too. Just do 'echo x - -' where x corresponds to the PMP port number. On older versions it's 'echo - x -'. Also, this again doesn't allow the user to specify soft or hard reset - I believe rescanning forces hardresets. Fully implementing the SATA spec, which should eventually be done anyway, seems like a cleaner solution. It defaults to softreset. -- 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
Quite frankly, I don't quite understand where you get those enormous balls you have, that you can then talk about how ugly it is to just add a = 0 that shuts up a compiler warning. That's the _least_ ugly part of the whole damn function! The clanking when I walk annoys people in the office too... But you're right. It is stupid of me to make such a big deal about this. My excuse is that I've seen those warnings so many times and actually given them more thought than they deserve, and I really felt that Jeff's change makes the admittedly already ugly code a tiny little bit worse. Anyway, here's a totally untested cleanup that compiles but probably doesn't work, because I didn't check that I did the right thing with all the pointer arithmetic (ie when I change wqe to a real structure pointer instead of just a void *, maybe I left some pointer arithmetic around that expected it to work as a byte pointer, but now really works on the whole structure size instead). Given that you took the time to do this, I'll get the patch into a working state and apply it. And maybe split it into reviewable chunks while I'm at it ;) Thanks, Roland - 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: Recommendations on user-space soft/hardreset
On 7/17/07, Tejun Heo [EMAIL PROTECTED] wrote: Yasha Okshtein wrote: Well ideally this should work with a port multiplier to soft/hardreset only one disk behind the multiplier. You can do that too. Just do 'echo x - -' where x corresponds to the PMP port number. On older versions it's 'echo - x -'. Nice, thanks! Also, this again doesn't allow the user to specify soft or hard reset - I believe rescanning forces hardresets. Fully implementing the SATA spec, which should eventually be done anyway, seems like a cleaner solution. It defaults to softreset. That still leaves the original problem of hardresets :) - yasha - 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
compile error if CONFIG_BLOCK not enabled related to linux/ide.h include
M: [EMAIL PROTECTED] L: linux-ide@vger.kernel.org We get the following compile error if CONFIG_BLOCK isn't enabled: CC arch/powerpc/kernel/setup_32.o In file included from arch/powerpc/kernel/setup_32.c:14: include/linux/ide.h:558: error: expected specifier-qualifier-list before 'request_queue_t' include/linux/ide.h:696: warning: 'struct request' declared inside parameter list include/linux/ide.h:696: warning: its scope is only this definition or declaration, which is probably not what you want include/linux/ide.h:820: warning: 'struct request' declared inside parameter list include/linux/ide.h:853: error: field 'wrq' has incomplete type include/linux/ide.h:1205: error: expected ')' before '*' token make[1]: *** [arch/powerpc/kernel/setup_32.o] Error 1 make: *** [arch/powerpc/kernel] Error 2 What I'm trying to figure out is if include/linux/ide.h should be wrapped in a #if defined(CONFIG_IDE) || defined(CONFIG_IDE_MODULE) or if there is some other desired way to handle this. This seems to stem from the fact that include/linux/blkdev.h is wrapped in a CONFIG_BLOCK and thus request_queue_t isn't always defined. - k - 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: Recommendations on user-space soft/hardreset
Yasha Okshtein wrote: It defaults to softreset. That still leaves the original problem of hardresets :) Yeah, I know, but with all other things served well by SCSI scan request, I don't really think another reset request mechanism is too compelling. After all, why do you care whether SRST or HRST is used? It's a reset anyway. -- 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: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Roland Dreier wrote: Anyway, here's a totally untested cleanup that compiles but probably doesn't work, because I didn't check that I did the right thing with all the pointer arithmetic (ie when I change wqe to a real structure pointer instead of just a void *, maybe I left some pointer arithmetic around that expected it to work as a byte pointer, but now really works on the whole structure size instead). Given that you took the time to do this, I'll get the patch into a working state and apply it. And maybe split it into reviewable chunks while I'm at it ;) Hey, I appreciate it, but I really do have to warn you that I did this all blind, and just meant for it to be a I think this kind of direction is more productive thing. I'm not going to guarantee that it works at all. I spent more time than I really wanted to on actually making sure the end result even compiles (quite often, I'm perfectly happy to just send out pseudo-code to indicate what I think should be done), but maybe I shouldn't have done that, just so that nobody thinks that the patch is necessarily going to *work*. In other words, it's a totally mindless cleanup and re-factorization. It *may* work, but quite frankly, I did it just for that one email, and spent the absolutly minimum possible time thinking about it. As a result, I really think it needs some serious looking over. I also think that my new handle_raddr_seg() function could itself be split up into subfunctions along the switch() subcases. So not only wasn't it meant to be guaranteed correct, but it wasn't even meant to be seen as the best possible final situation: it could be - and should be - factored out even more (and the same goes for a lot of the other functions in that file that I didn't really look at, just glance and notice that they have some of the same problem patterns). Linus - 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
