Re: [PATCH 2/4] libata: Clear tf before doing request sense
Tejun Heo wrote:
> Albert Lee wrote:
>
>> diff -Nrup 01_hsm_st/drivers/ata/libata-eh.c
>> 02_aopen_rs/drivers/ata/libata-eh.c
>> --- 01_hsm_st/drivers/ata/libata-eh.c2007-03-23 16:56:13.0
>> +0800
>> +++ 02_aopen_rs/drivers/ata/libata-eh.c2007-03-31
>> 01:11:01.0 +0800
>> @@ -991,18 +991,19 @@ static unsigned int atapi_eh_request_sen
>>
>> DPRINTK("ATAPI request sense\n");
>>
>> -ata_tf_init(dev, &tf);
>> -
>> /* FIXME: is this needed? */
>> memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
>>
>> -/* XXX: why tf_read here? */
>> +/* read error register to initialize sense_buf */
>> ap->ops->tf_read(ap, &tf);
>>
>> /* fill these in, for the case where they are -not- overwritten */
>> sense_buf[0] = 0x70;
>> sense_buf[2] = tf.feature >> 4;
>
>
> Oh, now I see why it's there. Thanks for spotting this. We don't need
> tf_read here, you can simply use the value in qc->result_tf.feature for
> this purpose.
>
Thanks for the advice. Will revise this patch.
--
albert
-
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/4] libata: Limit ATAPI DMA to R/W commands only for TORiSAN DVD drives
Albert Lee wrote: patch 4/4: Limit ATAPI DMA to R/W commands only for TORiSAN DRD-N216 DVD-ROM drives (http://bugzilla.kernel.org/show_bug.cgi?id=6710) Signed-off-by: Albert Lee <[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 3/4] libata: Limit max sector to 240 for TORiSAN DVD drives
Albert Lee wrote: patch 3/4: The TORiSAN drive locks up when max sector == 256. Limit max sector to 240 for TORiSAN DRD-N216 drives. (http://bugzilla.kernel.org/show_bug.cgi?id=6710) Signed-off-by: Albert Lee <[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 2/4] libata: Clear tf before doing request sense
Albert Lee wrote:
diff -Nrup 01_hsm_st/drivers/ata/libata-eh.c 02_aopen_rs/drivers/ata/libata-eh.c
--- 01_hsm_st/drivers/ata/libata-eh.c 2007-03-23 16:56:13.0 +0800
+++ 02_aopen_rs/drivers/ata/libata-eh.c 2007-03-31 01:11:01.0 +0800
@@ -991,18 +991,19 @@ static unsigned int atapi_eh_request_sen
DPRINTK("ATAPI request sense\n");
- ata_tf_init(dev, &tf);
-
/* FIXME: is this needed? */
memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
- /* XXX: why tf_read here? */
+ /* read error register to initialize sense_buf */
ap->ops->tf_read(ap, &tf);
/* fill these in, for the case where they are -not- overwritten */
sense_buf[0] = 0x70;
sense_buf[2] = tf.feature >> 4;
Oh, now I see why it's there. Thanks for spotting this. We don't need
tf_read here, you can simply use the value in qc->result_tf.feature for
this purpose.
--
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 4/4] libata: Limit ATAPI DMA to R/W commands only for TORiSAN DVD drives
patch 4/4:
Limit ATAPI DMA to R/W commands only for TORiSAN DRD-N216 DVD-ROM drives
(http://bugzilla.kernel.org/show_bug.cgi?id=6710)
Signed-off-by: Albert Lee <[EMAIL PROTECTED]>
---
I guess maybe other commands like READ_DVD_STRUCTURE and READ_CD also
work under ATAPI DMA, but not confirmed by Vlad yet. So, allow ATAPI DMA to
only for Read/Write here for safty.
Patch against libata-dev tree, for your review, thanks.
diff -Nrup 03_max_sect/drivers/ata/libata-core.c
04_dma_rw_only/drivers/ata/libata-core.c
--- 03_max_sect/drivers/ata/libata-core.c 2007-03-31 14:11:33.0
+0800
+++ 04_dma_rw_only/drivers/ata/libata-core.c2007-03-31 14:27:47.0
+0800
@@ -1787,6 +1787,10 @@ int ata_dev_configure(struct ata_device
if (ata_device_blacklisted(dev) & ATA_HORKAGE_MAX_SEC_240)
dev->max_sectors = min(ATA_MAX_SECTORS_240, dev->max_sectors);
+ /* limit ATAPI DMA to R/W commands only */
+ if (ata_device_blacklisted(dev) & ATA_HORKAGE_DMA_RW_ONLY)
+ dev->horkage |= ATA_HORKAGE_DMA_RW_ONLY;
+
if (ap->ops->dev_config)
ap->ops->dev_config(ap, dev);
@@ -3356,7 +3360,8 @@ static const struct ata_blacklist_entry
{ "SAMSUNG CD-ROM SN-124","N001", ATA_HORKAGE_NODMA },
/* Weird ATAPI devices */
- { "TORiSAN DVD-ROM DRD-N216", NULL, ATA_HORKAGE_MAX_SEC_240 },
+ { "TORiSAN DVD-ROM DRD-N216", NULL, ATA_HORKAGE_MAX_SEC_240 |
+ ATA_HORKAGE_DMA_RW_ONLY },
/* Devices we expect to fail diagnostics */
@@ -3676,6 +3681,26 @@ int ata_check_atapi_dma(struct ata_queue
struct ata_port *ap = qc->ap;
int rc = 0; /* Assume ATAPI DMA is OK by default */
+ /* some drives can only do ATAPI DMA on read/write */
+ if (unlikely(qc->dev->horkage & ATA_HORKAGE_DMA_RW_ONLY)) {
+ struct scsi_cmnd *cmd = qc->scsicmd;
+ u8 *scsicmd = cmd->cmnd;
+
+ switch (scsicmd[0]) {
+ case READ_10:
+ case WRITE_10:
+ case READ_12:
+ case WRITE_12:
+ case READ_6:
+ case WRITE_6:
+ /* atapi dma maybe ok */
+ break;
+ default:
+ /* turn off atapi dma */
+ return 1;
+ }
+ }
+
if (ap->ops->check_atapi_dma)
rc = ap->ops->check_atapi_dma(qc);
diff -Nrup 03_max_sect/include/linux/libata.h
04_dma_rw_only/include/linux/libata.h
--- 03_max_sect/include/linux/libata.h 2007-03-31 14:07:49.0 +0800
+++ 04_dma_rw_only/include/linux/libata.h 2007-03-31 14:08:31.0
+0800
@@ -312,6 +312,7 @@ enum {
ATA_HORKAGE_NODMA = (1 << 1), /* DMA problems */
ATA_HORKAGE_NONCQ = (1 << 2), /* Don't use NCQ */
ATA_HORKAGE_MAX_SEC_240 = (1 << 3), /* Limit max sects below 256 */
+ ATA_HORKAGE_DMA_RW_ONLY = (1 << 4), /* ATAPI DMA for RW only */
};
enum hsm_task_states {
-
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/4] libata: reorder HSM_ST_FIRST for easier decoding
Albert Lee wrote: patch 1/4: Reorder HSM_ST_FIRST, such that the task state transition is easier decoded with human eyes. Signed-off-by: Albert Lee <[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
[PATCH 3/4] libata: Limit max sector to 240 for TORiSAN DVD drives
patch 3/4:
The TORiSAN drive locks up when max sector == 256.
Limit max sector to 240 for TORiSAN DRD-N216 drives.
(http://bugzilla.kernel.org/show_bug.cgi?id=6710)
Signed-off-by: Albert Lee <[EMAIL PROTECTED]>
---
Patch against libata-dev tree, for your review, thanks.
diff -Nrup 02_aopen_rs/drivers/ata/libata-core.c
03_max_sect/drivers/ata/libata-core.c
--- 02_aopen_rs/drivers/ata/libata-core.c 2007-03-23 16:56:13.0
+0800
+++ 03_max_sect/drivers/ata/libata-core.c 2007-03-31 14:11:33.0
+0800
@@ -1784,6 +1784,9 @@ int ata_dev_configure(struct ata_device
dev->max_sectors = ATA_MAX_SECTORS;
}
+ if (ata_device_blacklisted(dev) & ATA_HORKAGE_MAX_SEC_240)
+ dev->max_sectors = min(ATA_MAX_SECTORS_240, dev->max_sectors);
+
if (ap->ops->dev_config)
ap->ops->dev_config(ap, dev);
@@ -3352,6 +3355,9 @@ static const struct ata_blacklist_entry
{ "_NEC DV5800A", NULL, ATA_HORKAGE_NODMA },
{ "SAMSUNG CD-ROM SN-124","N001", ATA_HORKAGE_NODMA },
+ /* Weird ATAPI devices */
+ { "TORiSAN DVD-ROM DRD-N216", NULL, ATA_HORKAGE_MAX_SEC_240 },
+
/* Devices we expect to fail diagnostics */
/* Devices where NCQ should be avoided */
diff -Nrup 02_aopen_rs/include/linux/ata.h 03_max_sect/include/linux/ata.h
--- 02_aopen_rs/include/linux/ata.h 2007-03-23 16:56:24.0 +0800
+++ 03_max_sect/include/linux/ata.h 2007-03-31 10:06:53.0 +0800
@@ -40,6 +40,7 @@ enum {
ATA_MAX_DEVICES = 2,/* per bus/port */
ATA_MAX_PRD = 256, /* we could make these 256/256 */
ATA_SECT_SIZE = 512,
+ ATA_MAX_SECTORS_240 = 240,
ATA_MAX_SECTORS = 256,
ATA_MAX_SECTORS_LBA48 = 65535,/* TODO: 65536? */
diff -Nrup 02_aopen_rs/include/linux/libata.h 03_max_sect/include/linux/libata.h
--- 02_aopen_rs/include/linux/libata.h 2007-03-26 12:33:28.0 +0800
+++ 03_max_sect/include/linux/libata.h 2007-03-31 14:07:49.0 +0800
@@ -311,6 +311,7 @@ enum {
ATA_HORKAGE_DIAGNOSTIC = (1 << 0), /* Failed boot diag */
ATA_HORKAGE_NODMA = (1 << 1), /* DMA problems */
ATA_HORKAGE_NONCQ = (1 << 2), /* Don't use NCQ */
+ ATA_HORKAGE_MAX_SEC_240 = (1 << 3), /* Limit max sects below 256 */
};
enum hsm_task_states {
-
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/4] libata: Clear tf before doing request sense
patch 2/4:
Clear tf before doing request sense.
This fixes the AOpen 56X/AKH timeout problem.
(http://bugzilla.kernel.org/show_bug.cgi?id=8244)
Signed-off-by: Albert Lee <[EMAIL PROTECTED]>
---
Patch against libata-dev tree, for your review, thanks.
diff -Nrup 01_hsm_st/drivers/ata/libata-eh.c 02_aopen_rs/drivers/ata/libata-eh.c
--- 01_hsm_st/drivers/ata/libata-eh.c 2007-03-23 16:56:13.0 +0800
+++ 02_aopen_rs/drivers/ata/libata-eh.c 2007-03-31 01:11:01.0 +0800
@@ -991,18 +991,19 @@ static unsigned int atapi_eh_request_sen
DPRINTK("ATAPI request sense\n");
- ata_tf_init(dev, &tf);
-
/* FIXME: is this needed? */
memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
- /* XXX: why tf_read here? */
+ /* read error register to initialize sense_buf */
ap->ops->tf_read(ap, &tf);
/* fill these in, for the case where they are -not- overwritten */
sense_buf[0] = 0x70;
sense_buf[2] = tf.feature >> 4;
+ /* some devices time out if garbage left in tf */
+ ata_tf_init(dev, &tf);
+
memset(cdb, 0, ATAPI_CDB_LEN);
cdb[0] = REQUEST_SENSE;
cdb[4] = SCSI_SENSE_BUFFERSIZE;
-
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/4] libata: reorder HSM_ST_FIRST for easier decoding
patch 1/4:
Reorder HSM_ST_FIRST, such that the task state transition is easier decoded
with human eyes.
Signed-off-by: Albert Lee <[EMAIL PROTECTED]>
---
Patch against libata-dev tree, for your review, thanks.
diff -Nrup 00_libata-dev.ori/include/linux/libata.h
01_hsm_st/include/linux/libata.h
--- 00_libata-dev.ori/include/linux/libata.h2007-03-23 16:56:24.0
+0800
+++ 01_hsm_st/include/linux/libata.h2007-03-26 12:33:28.0 +0800
@@ -315,11 +315,11 @@ enum {
enum hsm_task_states {
HSM_ST_IDLE,/* no command on going */
+ HSM_ST_FIRST, /* (waiting the device to)
+ write CDB or first data block */
HSM_ST, /* (waiting the device to) transfer data */
HSM_ST_LAST,/* (waiting the device to) complete command */
HSM_ST_ERR, /* error */
- HSM_ST_FIRST, /* (waiting the device to)
- write CDB or first data block */
};
enum ata_completion_errors {
-
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 0/4] libata: Workaround/fixes for ATAPI devices
patch 1/4: Reorder HSM_ST_FIRST patch 2/4: Clear tf before doing request sense patch 3/4: Limit max sector to 240 for TORiSAN DVD drives patch 4/4: Limit ATAPI DMA to R/W commands only for TORiSAN DVD drives - 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 bugfix: preserve LBA bit for HDIO_DRIVE_TASK
Mark Lord wrote: > Ideally, this would go into linux-2.6.21. > > Preserve the LBA bit in the DevSel/Head register for HDIO_DRIVE_TASK. > > Signed-off-by: Mark Lord <[EMAIL PROTECTED]> > --- > --- linux/drivers/ata/libata-scsi.c.orig2007-03-21 > 13:35:02.0 -0400 > +++ linux/drivers/ata/libata-scsi.c2007-03-30 17:40:58.0 -0400 > @@ -333,7 +333,7 @@ > scsi_cmd[8] = args[3]; > scsi_cmd[10] = args[4]; > scsi_cmd[12] = args[5]; > -scsi_cmd[13] = args[6] & 0x0f; > +scsi_cmd[13] = args[6] & 0x4f; > scsi_cmd[14] = args[0]; > > /* Good values for timeout and retries? Values below IDE seems to be just overriding devsel (0x10) and leaving the rest alone. Maybe we should do (args[6] & ~0x10) here? Or is it safer this way? 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: [RFD driver-core] Lifetime problems of the current driver model
Tejun Heo wrote:
> Cornelia Huck wrote:
>> On Sat, 31 Mar 2007 00:08:19 +0900,
>> Tejun Heo <[EMAIL PROTECTED]> wrote:
>>
>>> (3) make sure all existing kobjects are released by module exit function.
>>>
>>> For example, let's say there is a hypothetical disk device /dev/dk0
>>> driven by a hypothetical driver mydrv. /dev/dk0 is represented like the
>>> following in the sysfs tree.
>>>
>>> /sys/devices/pci:00/:00:1f.0/dk0/{myknob0,myknob1}
>>>
>>> Owner of both attrs myknob0 and myknob1 is mydrv and opening either
>>> increases the reference counts of dk0 and mydrv and closing does the
>>> opposite.
>>>
>>> * When there is no opener of either knob and the /dev/dk0 isn't used by
>>> anyone. Reference count of dk0 is 1, mydrv 0.
>> Hm, but as long as dk0 is registered, it can be looked up and someone
>> could get a reference on it.
>
> Yeah, exactly. That's why any getting any kobject reference backed by a
> module must be accompanied by try_module_get().
>
> int mydrv_get_dk(struct dk *dk)
> {
> rc = try_module_get(mydrv);
> if (rc)
> return rc;
> kobject_get(&dk->kobj);
> return 0;
> }
And one more thing just in case. In the above code, try_module_get()
and kobject_get() must be and is atomic w.r.t. try_stop_module().
That's why we do the following.
stop_machine_run(__try_stop_module, &sref, NR_CPUS);.
--
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: [RFD driver-core] Lifetime problems of the current driver model
Cornelia Huck wrote:
> On Sat, 31 Mar 2007 00:08:19 +0900,
> Tejun Heo <[EMAIL PROTECTED]> wrote:
>
>> (3) make sure all existing kobjects are released by module exit function.
>>
>> For example, let's say there is a hypothetical disk device /dev/dk0
>> driven by a hypothetical driver mydrv. /dev/dk0 is represented like the
>> following in the sysfs tree.
>>
>> /sys/devices/pci:00/:00:1f.0/dk0/{myknob0,myknob1}
>>
>> Owner of both attrs myknob0 and myknob1 is mydrv and opening either
>> increases the reference counts of dk0 and mydrv and closing does the
>> opposite.
>>
>> * When there is no opener of either knob and the /dev/dk0 isn't used by
>> anyone. Reference count of dk0 is 1, mydrv 0.
>
> Hm, but as long as dk0 is registered, it can be looked up and someone
> could get a reference on it.
Yeah, exactly. That's why any getting any kobject reference backed by a
module must be accompanied by try_module_get().
int mydrv_get_dk(struct dk *dk)
{
rc = try_module_get(mydrv);
if (rc)
return rc;
kobject_get(&dk->kobj);
return 0;
}
>> * User issues rmmod mydrv. As mydrv's reference count is zero, unload
>> proceeds and mydrv's exit function is called.
>>
>> * mydrv's exit function looks like the following.
>>
>> mydrv_exit()
>> {
>> sysfs_remove_file(dk0, myknob0);
>> sysfs_remove_file(dk1, myknob1);
>> device_del(dk0);
>> deinit controller;
>> release all resources;
>> }
>>
>> The device_del(dk0) drops dk0's reference count to zero and its
>> ->release is invoked immediately.
>
> And here is the problem if someone else still has a reference. The
> module will be unloaded, but ->release will not be called until the
> "someone else" gives up the reference...
Exactly, in that case, module reference count must not be zero. You and
I are saying the same thing. Why are we running in circle?
--
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: [3/4] 2.6.21-rc5: known regressions (v2)
On 3/31/07, Adrian Bunk <[EMAIL PROTECTED]> wrote: Subject: ThinkPad doesn't resume from suspend to RAM References : http://lkml.org/lkml/2007/2/27/80 http://lkml.org/lkml/2007/2/28/348 Submitter : Jens Axboe <[EMAIL PROTECTED]> Jeff Chua <[EMAIL PROTECTED]> Status : unknown Fixed with CONFIG_NO_HZ unset and patch from Maxim (http://lkml.org/lkml/2007/3/29/108). Thanks, 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: [1/4] 2.6.21-rc5: known regressions (v2)
On Fri, Mar 30, 2007 at 11:32:09PM +0200, Adrian Bunk wrote: > > Subject: kernels fail to boot with drives on ATIIXP controller > (ACPI/IRQ related) > References : https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229621 > http://lkml.org/lkml/2007/3/4/257 > Submitter : Michal Jaegermann <[EMAIL PROTECTED]> > Status : unknown I have now even better one with pata_via. A kernel, which for all practical purposes is 2.6.21-rc5, not only refuses to boot (and I cannot find some option combination which would allow me to do so anyway) but simply refuses to read _any_ data from a media. This included a partitioning information. Earlier kernel on the same hardware boots without raising any fuss. Details are collected as https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234650 Michal - 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 bugfix: preserve LBA bit for HDIO_DRIVE_TASK
Ideally, this would go into linux-2.6.21. Preserve the LBA bit in the DevSel/Head register for HDIO_DRIVE_TASK. Signed-off-by: Mark Lord <[EMAIL PROTECTED]> --- --- linux/drivers/ata/libata-scsi.c.orig2007-03-21 13:35:02.0 -0400 +++ linux/drivers/ata/libata-scsi.c 2007-03-30 17:40:58.0 -0400 @@ -333,7 +333,7 @@ scsi_cmd[8] = args[3]; scsi_cmd[10] = args[4]; scsi_cmd[12] = args[5]; - scsi_cmd[13] = args[6] & 0x0f; + scsi_cmd[13] = args[6] & 0x4f; scsi_cmd[14] = args[0]; /* Good values for timeout and retries? Values below -- Mark Lord Real-Time Remedies Inc. [EMAIL PROTECTED] - 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: [1/4] 2.6.21-rc5: known regressions (v2)
On Fri, Mar 30, 2007 at 11:32:09PM +0200, Adrian Bunk wrote: > Subject: hung bootup in various drivers > References : http://lkml.org/lkml/2007/3/30/68 > Submitter : Ingo Molnar <[EMAIL PROTECTED]> > Handled-By : Ingo Molnar <[EMAIL PROTECTED]> > Greg KH <[EMAIL PROTECTED]> > Kay Sievers <[EMAIL PROTECTED]> > Status : problem is being discussed Note, this should probably read: hung bootup for drivers built into the kernel, that fail their module_init() call. A much smaller minority of cases :) 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
[3/4] 2.6.21-rc5: known regressions (v2)
This email lists some known regressions in Linus' tree compared to 2.6.20. If you find your name in the Cc header, you are either submitter of one of the bugs, maintainer of an affectected subsystem or driver, a patch of you caused a breakage or I'm considering you in any other way possibly involved with one or more of these issues. Due to the huge amount of recipients, please trim the Cc when answering. Subject: ThinkPad X60: resume no longer works (PCI related?) References : http://lkml.org/lkml/2007/3/13/3 Submitter : Dave Jones <[EMAIL PROTECTED]> Jeremy Fitzhardinge <[EMAIL PROTECTED]> Caused-By : PCI merge commit 78149df6d565c36675463352d0bfeb02b7a7 Handled-By : Eric W. Biederman <[EMAIL PROTECTED]> Rafael J. Wysocki <[EMAIL PROTECTED]> Status : problem is being debugged Subject: Suspend to RAM doesn't work anymore (ACPI?) References : http://lkml.org/lkml/2007/3/19/128 http://bugzilla.kernel.org/show_bug.cgi?id=8247 Submitter : Tobias Doerffel <[EMAIL PROTECTED]> Handled-By : Rafael J. Wysocki <[EMAIL PROTECTED]> Len Brown <[EMAIL PROTECTED]> Status : problem is being debugged Subject: SATA breakage on resume References : http://lkml.org/lkml/2007/3/7/233 Submitter : Thomas Gleixner <[EMAIL PROTECTED]> Soeren Sonnenburg <[EMAIL PROTECTED]> Status : unknown Subject: resume from RAM corrupts vesafb console References : http://lkml.org/lkml/2007/3/26/76 Submitter : Marcus Better <[EMAIL PROTECTED]> Handled-By : Pavel Machek <[EMAIL PROTECTED]> Status : problem is being debugged Subject: ThinkPad doesn't resume from suspend to RAM References : http://lkml.org/lkml/2007/2/27/80 http://lkml.org/lkml/2007/2/28/348 Submitter : Jens Axboe <[EMAIL PROTECTED]> Jeff Chua <[EMAIL PROTECTED]> Status : unknown Subject: MacBook Core Duo: suspend to memory wakeup hang References : http://bugzilla.kernel.org/show_bug.cgi?id=8272 Submitter : Mike Harris <[EMAIL PROTECTED]> Status : unknown Subject: suspend to disk hangs (skge) References : http://lkml.org/lkml/2007/3/27/212 Submitter : Michal Piotrowski <[EMAIL PROTECTED]> Caused-By : Stephen Hemminger <[EMAIL PROTECTED]> commit a504e64ab42bcc27074ea37405d06833ed6e0820 Status : unknown Subject: suspend to disk hangs (microcode driver) References : http://lkml.org/lkml/2007/3/16/126 Submitter : Maxim Levitsky <[EMAIL PROTECTED]> Caused-By : Rafael J. Wysocki <[EMAIL PROTECTED]> commit e3c7db621bed4afb8e231cb005057f2feb5db557 commit ed746e3b18f4df18afa3763155972c5835f284c5 commit 259130526c267550bc365d3015917d90667732f1 Handled-By : Rafael J. Wysocki <[EMAIL PROTECTED]> Patch : http://lkml.org/lkml/2007/3/25/71 Status : patch available - 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
[1/4] 2.6.21-rc5: known regressions (v2)
This email lists some known regressions in Linus' tree compared to 2.6.20. If you find your name in the Cc header, you are either submitter of one of the bugs, maintainer of an affectected subsystem or driver, a patch of you caused a breakage or I'm considering you in any other way possibly involved with one or more of these issues. Due to the huge amount of recipients, please trim the Cc when answering. Subject: crashes in KDE References : http://bugzilla.kernel.org/show_bug.cgi?id=8157 Submitter : Oliver Pinter <[EMAIL PROTECTED]> Status : unknown Subject: hung bootup in various drivers References : http://lkml.org/lkml/2007/3/30/68 Submitter : Ingo Molnar <[EMAIL PROTECTED]> Handled-By : Ingo Molnar <[EMAIL PROTECTED]> Greg KH <[EMAIL PROTECTED]> Kay Sievers <[EMAIL PROTECTED]> Status : problem is being discussed Subject: kernels fail to boot with drives on ATIIXP controller (ACPI/IRQ related) References : https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229621 http://lkml.org/lkml/2007/3/4/257 Submitter : Michal Jaegermann <[EMAIL PROTECTED]> Status : unknown Subject: NCQ problem with ahci and Hitachi drive References : http://lkml.org/lkml/2007/3/4/178 http://lkml.org/lkml/2007/3/9/475 http://lkml.org/lkml/2007/2/22/8 Submitter : Mathieu Bérard <[EMAIL PROTECTED]> Handled-By : Tejun Heo <[EMAIL PROTECTED]> Patch : http://lkml.org/lkml/2007/2/22/8 Status : possible patch available - 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: [RFD driver-core] Lifetime problems of the current driver model
On Sat, 31 Mar 2007 00:08:19 +0900,
Tejun Heo <[EMAIL PROTECTED]> wrote:
> (3) make sure all existing kobjects are released by module exit function.
>
> For example, let's say there is a hypothetical disk device /dev/dk0
> driven by a hypothetical driver mydrv. /dev/dk0 is represented like the
> following in the sysfs tree.
>
> /sys/devices/pci:00/:00:1f.0/dk0/{myknob0,myknob1}
>
> Owner of both attrs myknob0 and myknob1 is mydrv and opening either
> increases the reference counts of dk0 and mydrv and closing does the
> opposite.
>
> * When there is no opener of either knob and the /dev/dk0 isn't used by
> anyone. Reference count of dk0 is 1, mydrv 0.
Hm, but as long as dk0 is registered, it can be looked up and someone
could get a reference on it.
>
> * User issues rmmod mydrv. As mydrv's reference count is zero, unload
> proceeds and mydrv's exit function is called.
>
> * mydrv's exit function looks like the following.
>
> mydrv_exit()
> {
> sysfs_remove_file(dk0, myknob0);
> sysfs_remove_file(dk1, myknob1);
> device_del(dk0);
> deinit controller;
> release all resources;
> }
>
> The device_del(dk0) drops dk0's reference count to zero and its
> ->release is invoked immediately.
And here is the problem if someone else still has a reference. The
module will be unloaded, but ->release will not be called until the
"someone else" gives up the reference...
-
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: [RFD driver-core] Lifetime problems of the current driver model
On Fri, 2007-03-30 at 22:38 +0900, Tejun Heo wrote: > My plan was to make sysfs more independent from struct device/kobject. > e.g. Something like the following. That's sort of what I was reaching for too ... it just looks to me that all the sysfs glue is in kobject, so they make a good candidate for the pure sysfs objects. Whatever we do, there has to be breakable cross linking between the driver's tree and the sysfs representation ... of course, nasty things like ksets get in the way of considering the objects as separate, sigh. > * kobject_get() on attr/symlink creation > * open() doesn't need extra kobject reference > * deleting a node makes it release the kobject reference and the kobject > pointer is nullified. To do this, you'll have to move at least the dentry out of the kobject, I think. > This way the sysfs reference counting can be put completely out of > picture without impacting the rest of code. Handling symlink and > suicidal attributes might need some extra attention but I think they can > be done. True ... but you'll also have to implement something within sysfs to do refcounting ... it needs to know how long to keep its nodes around. > In the long term, I think sysfs should be independent from driver model > and kobject such that an entity which wants to use sysfs but is not a > device doesn't have to dance with kobject just to use sysfs. I agree ... the question is just how to do it. I'd favour trying to separate kobject and struct device for this ... move all the sysfs stuff into kobject and device only stuff into struct device ... but that would get us into disentangling the ksets, which, on balance, isn't going to be fun ... James - 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: [RFD driver-core] Lifetime problems of the current driver model
On 3/30/07, James Bottomley <[EMAIL PROTECTED]> wrote: On Fri, 2007-03-30 at 09:15 -0400, Dmitry Torokhov wrote: > If you want to manage lifetime rules independently you might want to > not embed struct device into you subsystems objects but attach them > via pointers and use device_create(). Now that we orphan sysfs access > upon unregistering device this will severe all ties from driver core > to your system once you start teardown of a device and you should be > in clear. But that wouldn't really help ... all objects have lifetimes. What Tejun is pointing out is that we have two different lifetime requirements: driver internal (and subsystem) objects and sysfs objects ... Exactly. If driver-private data structures have different lifetime than device abstractions (like they seem to have with scsi and ide) then you split them and refcount separately. We are just arguing where to put the line. You want to split devices and kobjects and rework infrastructure and I am saying that we already have infrastructure we need and that split shoud be between generic device structure and driver-managed device structure. -- Dmitry - 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: [RFD driver-core] Lifetime problems of the current driver model
On Fri, 2007-03-30 at 09:15 -0400, Dmitry Torokhov wrote: > If you want to manage lifetime rules independently you might want to > not embed struct device into you subsystems objects but attach them > via pointers and use device_create(). Now that we orphan sysfs access > upon unregistering device this will severe all ties from driver core > to your system once you start teardown of a device and you should be > in clear. But that wouldn't really help ... all objects have lifetimes. What Tejun is pointing out is that we have two different lifetime requirements: driver internal (and subsystem) objects and sysfs objects ... we still need something embedded in the driver objects, so it may as well be struct device ... trying to manage struct device outside of the driver objects would turn into another nasty refcounting problem. The struct device is usually the generic abstraction of the specific structure it's embedded in, so I think it really does make sense to keep these pieces together. > However there are simpler subsystems (input, serio, etc.) where there > is only one core module which provides services to device drivers and > handles registration and deregistration. For such substustems it makes > sense to embed struct devices and manage lifetime for all components > at once. James - 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: [RFD driver-core] Lifetime problems of the current driver model
On Fri, Mar 30, 2007 at 10:38:00PM +0900, Tejun Heo wrote: > James Bottomley wrote: > > On Fri, 2007-03-30 at 18:43 +0900, Tejun Heo wrote: > >> Orphaning sysfs nodes on unregistration is a big step in this > >> direction. With sysfs reference counting out of the picture, > >> implementing 'disconnect immediate' interface only on a few components > >> (including request_queue) should suffice for most block device > >> drivers. I'm not familiar with other drivers but I don't think > >> they'll be very different. > > > > I agree with this statement. The big question in my mind is how do we > > do it? > > > > The essential problem, and the reason why the lifetime rules are > > entangled is that fundamentally, sysfs entries are managed by kobjects. > > The things the device drivers are interested in is struct device, which > > is usually embedded in driver data structures. Unfortunately, the > > required kobject is usually embedded in struct device meaning that the > > relevant driver structure cannot be freed while the sysfs entry still > > exists. > > > > It seems to me that the only way to Orphan the sysfs entries is to > > separate the kobject and the struct device so their lifetime rules are > > no longer entangled. Then we can free the driver structure with the > > embedded struct device while still keeping the necessary kobject around > > to perform orphaned sysfs operations. > > > > So it seems to me that what we need to do is to give struct device its > > own kref and a pointer to a kobject. Then we can manage the separate > > lifetimes independently. The device would basically allocate and take a > > reference to the kobject on device_add() and relinquish it again (and > > null out the kobject pointer) on device_del(). The complexity here is > > that we now have to allocate the kobject somewhere else ... probably in > > device_add() (it doesn't come for free with the device structures). > > My plan was to make sysfs more independent from struct device/kobject. > e.g. Something like the following. > > * kobject_get() on attr/symlink creation > * open() doesn't need extra kobject reference > * deleting a node makes it release the kobject reference and the kobject > pointer is nullified. Hm, this sounds good, but I think it will be racy. Although separating the very tight tie between sysfs and kobject would be very good to have. Pat originally wanted to do that years ago, but he's now lost to the land of a million sheep... So I don't object to this goal at all. > This way the sysfs reference counting can be put completely out of > picture without impacting the rest of code. Handling symlink and > suicidal attributes might need some extra attention but I think they can > be done. Ok, I'll but I really want to see that code :) > In the long term, I think sysfs should be independent from driver model > and kobject such that an entity which wants to use sysfs but is not a > device doesn't have to dance with kobject just to use sysfs. I agree. 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
Re: [RFD driver-core] Lifetime problems of the current driver model
On Fri, Mar 30, 2007 at 06:43:02PM +0900, Tejun Heo wrote: > Hello, all. > > This document tries to describe lifetime problems of the current > device driver model primarily from the point view of device drivers > and establish consensus, or at least, start discussion about how to > solve these problems. This is primarily based on my experience with > IDE and SCSI layers and my knowledge on other drivers is limited, so I > might have generalized too aggressively. Feel free to point out. Very nice summary of the problems we are trying to address here, thanks for doing this. > 2. How do we solve this? > > 2-1. Rule #a and #b > > If you think about it, the impedance matching between rule #a and #b > should be done somewhere. Maybe doing it in each low level driver is > what's intended when the driver model is designed but it's > impractically painful and the reality reflects that by doing it in > midlayers. > > It's better if we do it in higher layers. For example, SCSI has a > mechanism to reject new requests and drain request_queue to allow low > level driver to just unregister an existing device. IDE currently > doesn't have such feature but it would need to do almost the same > thing to support hotplug. If request_queue just had a feature to > drain and kill itself, both SCSI and IDE could use it. It would be > simpler and less error-prone. On the other hand, if it's pushed > downward, it will cause much more pain in all the low level drivers. No, we should not push this kind of thing down to the lower drivers. In fact, my goal is to try to keep a "normal" driver writer from having to know anything about the driver model, except for how to add and remove a sysfs file that is bound to the device controlled by the driver. And for the majority of the busses, this is true, it's only people like you who are mucking around in the bus specific code that know how hard this whole thing is :) > Orphaning sysfs nodes on unregistration is a big step in this > direction. With sysfs reference counting out of the picture, > implementing 'disconnect immediate' interface only on a few components > (including request_queue) should suffice for most block device > drivers. I'm not familiar with other drivers but I don't think > they'll be very different. > > All in all, I'm hoping something like the following can be done in > device drivers, midlayer or low level. > > * For binding > > alloc resources; > init controller; > register to upper layers; > > * For unbinding > > unregister from upper layers; (no lingering references or objects) > deinit controller; > release resources; > > This basically nullifies lifetime rule #b from the POV of drivers. I agree, that should be our goal. And for almost all busses, this is true today, right? > 2-2. Rule #b and #c > > One way to solve this problem is to subordinate lifetime rule #b to > rule #c. Each kobject points to its owning module such that grabbing > a kobject automatically grabs the module. The problem with this > approach is that it requires wide update and makes kobject_get > heavier. Why would kobject_get become heavier? > Another more appealing approach is to do nothing. Solving the problem > between rule #a and #b in the way described above means virtually > nullifying rule #b. With rule #b gone, rule #c can't conflict with > it. IOW, no reference from upper layer would be remaining after a > driver unregisters itself from the upper layer - the module can be > safely unloaded. Yes, but you have to watch out for devices that add their own files with callbacks to that driver. That is a problem today that Oliver has tried to address with his recent patches. > 3. How do we get there from here? > > The current device driver model is used by most device drivers - huge > amount of code. We can't update them at once. No, the majority of the logic is in the busses, and we can update them pretty easily. > Fortunately, the > suggested solution can easily be implemented gradually. We can add > 'disconnect now' interface to upper driver interface one-by-one and > convert its users gradually. The existing reference counting > mechanism can be left alone and used to verify that the conversion is > correct by verifying reference count is 0 after unregistering. > > Once the conversion is complete (maybe after a year), we can remove > the reference counting mechanism from device driver interface. I'd be interested in seeing how we can do this. Do you have any specific patches started in this area? > 4. Some afterthoughts > > * Doing this would make module reference counting more flexible. > Instead of being confined by implementation, it can be used to > define when to allow and disallow module unloading. (you can't > unload a block driver module if a fs on it is mounted but sysfs > access doesn't matter.) Well, remember, module unloading is racy and unrecommended to use anyway :) Also, look at the network drivers who all hav
Re: [RFD driver-core] Lifetime problems of the current driver model
Cornelia Huck wrote:
> On Fri, 30 Mar 2007 22:58:39 +0900,
> Tejun Heo <[EMAIL PROTECTED]> wrote:
>
>> It's a little bit more convoluted than that. Module reference count of
>> zero doesn't indicate that there is no one referencing the module. It
>> just means that the module can be unloaded. ie. There still can be any
>> number of kobjects with release function backed by the module but as
>> long as all of them can be deleted and released by module exit function,
>> the module is unloadable at that point.
>>
>> IOW, module reference count does not count number of objects depending
>> on the module. It counts the number of active usages of those objects.
>
> We must make sure that the module is never deleted while there may be
> calls to ->release functions - the exit function can only return when
> all ->release calls have returned. This can be guaranteed if we (1)
> don't allow the module to unload if there are outstanding kobjects (we
> may need a "self destruct" knob then) or (2) make sure the ->release
> functions are outside of the module (see, for example,
> drivers/s390/s390_rdev.c).
(3) make sure all existing kobjects are released by module exit function.
For example, let's say there is a hypothetical disk device /dev/dk0
driven by a hypothetical driver mydrv. /dev/dk0 is represented like the
following in the sysfs tree.
/sys/devices/pci:00/:00:1f.0/dk0/{myknob0,myknob1}
Owner of both attrs myknob0 and myknob1 is mydrv and opening either
increases the reference counts of dk0 and mydrv and closing does the
opposite.
* When there is no opener of either knob and the /dev/dk0 isn't used by
anyone. Reference count of dk0 is 1, mydrv 0.
* User issues rmmod mydrv. As mydrv's reference count is zero, unload
proceeds and mydrv's exit function is called.
* mydrv's exit function looks like the following.
mydrv_exit()
{
sysfs_remove_file(dk0, myknob0);
sysfs_remove_file(dk1, myknob1);
device_del(dk0);
deinit controller;
release all resources;
}
The device_del(dk0) drops dk0's reference count to zero and its
->release is invoked immediately.
This method is widely used to allow modules to be unloaded even when
there still are valid objects if there's no active user.
> (Gah, that stuff is always giving me headaches. Sorry if I'm not making
> sense...)
Yeap, this is confusing. Hope my explanation makes 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: [RFD driver-core] Lifetime problems of the current driver model
On Fri, 30 Mar 2007 22:58:39 +0900, Tejun Heo <[EMAIL PROTECTED]> wrote: > It's a little bit more convoluted than that. Module reference count of > zero doesn't indicate that there is no one referencing the module. It > just means that the module can be unloaded. ie. There still can be any > number of kobjects with release function backed by the module but as > long as all of them can be deleted and released by module exit function, > the module is unloadable at that point. > > IOW, module reference count does not count number of objects depending > on the module. It counts the number of active usages of those objects. We must make sure that the module is never deleted while there may be calls to ->release functions - the exit function can only return when all ->release calls have returned. This can be guaranteed if we (1) don't allow the module to unload if there are outstanding kobjects (we may need a "self destruct" knob then) or (2) make sure the ->release functions are outside of the module (see, for example, drivers/s390/s390_rdev.c). (Gah, that stuff is always giving me headaches. Sorry if I'm not making sense...) - 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: [RFD driver-core] Lifetime problems of the current driver model
Cornelia Huck wrote: > On Fri, 30 Mar 2007 22:19:52 +0900, > Tejun Heo <[EMAIL PROTECTED]> wrote: > >>> Shouldn't getting/putting the module refcount be solely done in >>> kobject.c? Grab the module reference when the kobject is created and >>> release the module reference in kobject_cleanup() after the release >>> function has been called. This doesn't make kobject_get() heavier, and >>> it ensures we don't delete the module until after the last kobject it is >>> supposed to clean up has been released. >> If we do that, we wouldn't be able to unload a module if there is any >> kobject referencing it even when the node has no openers, so no easy way >> out there. :-( > > But we must not unload the module when there is still a kobject around > referencing a release function in the module, or we will oops if the > kobject is finally released. If needed, the module must clean up its > kobjects in its exit function. It's a little bit more convoluted than that. Module reference count of zero doesn't indicate that there is no one referencing the module. It just means that the module can be unloaded. ie. There still can be any number of kobjects with release function backed by the module but as long as all of them can be deleted and released by module exit function, the module is unloadable at that point. IOW, module reference count does not count number of objects depending on the module. It counts the number of active usages of those objects. -- 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: [RFD driver-core] Lifetime problems of the current driver model
On Fri, 30 Mar 2007 22:19:52 +0900, Tejun Heo <[EMAIL PROTECTED]> wrote: > > Shouldn't getting/putting the module refcount be solely done in > > kobject.c? Grab the module reference when the kobject is created and > > release the module reference in kobject_cleanup() after the release > > function has been called. This doesn't make kobject_get() heavier, and > > it ensures we don't delete the module until after the last kobject it is > > supposed to clean up has been released. > > If we do that, we wouldn't be able to unload a module if there is any > kobject referencing it even when the node has no openers, so no easy way > out there. :-( But we must not unload the module when there is still a kobject around referencing a release function in the module, or we will oops if the kobject is finally released. If needed, the module must clean up its kobjects in its exit function. - 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: [RFD driver-core] Lifetime problems of the current driver model
James Bottomley wrote: > On Fri, 2007-03-30 at 18:43 +0900, Tejun Heo wrote: >> Orphaning sysfs nodes on unregistration is a big step in this >> direction. With sysfs reference counting out of the picture, >> implementing 'disconnect immediate' interface only on a few components >> (including request_queue) should suffice for most block device >> drivers. I'm not familiar with other drivers but I don't think >> they'll be very different. > > I agree with this statement. The big question in my mind is how do we > do it? > > The essential problem, and the reason why the lifetime rules are > entangled is that fundamentally, sysfs entries are managed by kobjects. > The things the device drivers are interested in is struct device, which > is usually embedded in driver data structures. Unfortunately, the > required kobject is usually embedded in struct device meaning that the > relevant driver structure cannot be freed while the sysfs entry still > exists. > > It seems to me that the only way to Orphan the sysfs entries is to > separate the kobject and the struct device so their lifetime rules are > no longer entangled. Then we can free the driver structure with the > embedded struct device while still keeping the necessary kobject around > to perform orphaned sysfs operations. > > So it seems to me that what we need to do is to give struct device its > own kref and a pointer to a kobject. Then we can manage the separate > lifetimes independently. The device would basically allocate and take a > reference to the kobject on device_add() and relinquish it again (and > null out the kobject pointer) on device_del(). The complexity here is > that we now have to allocate the kobject somewhere else ... probably in > device_add() (it doesn't come for free with the device structures). My plan was to make sysfs more independent from struct device/kobject. e.g. Something like the following. * kobject_get() on attr/symlink creation * open() doesn't need extra kobject reference * deleting a node makes it release the kobject reference and the kobject pointer is nullified. This way the sysfs reference counting can be put completely out of picture without impacting the rest of code. Handling symlink and suicidal attributes might need some extra attention but I think they can be done. In the long term, I think sysfs should be independent from driver model and kobject such that an entity which wants to use sysfs but is not a device doesn't have to dance with kobject just to use sysfs. 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: [RFD driver-core] Lifetime problems of the current driver model
Cornelia Huck wrote: > On Fri, 30 Mar 2007 18:43:02 +0900, > Tejun Heo <[EMAIL PROTECTED]> wrote: > >> One way to solve this problem is to subordinate lifetime rule #b to >> rule #c. Each kobject points to its owning module such that grabbing >> a kobject automatically grabs the module. The problem with this >> approach is that it requires wide update and makes kobject_get >> heavier. > > Shouldn't getting/putting the module refcount be solely done in > kobject.c? Grab the module reference when the kobject is created and > release the module reference in kobject_cleanup() after the release > function has been called. This doesn't make kobject_get() heavier, and > it ensures we don't delete the module until after the last kobject it is > supposed to clean up has been released. If we do that, we wouldn't be able to unload a module if there is any kobject referencing it even when the node has no openers, so no easy way out there. :-( -- 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: [RFD driver-core] Lifetime problems of the current driver model
On Fri, 30 Mar 2007 18:43:02 +0900, Tejun Heo <[EMAIL PROTECTED]> wrote: > One way to solve this problem is to subordinate lifetime rule #b to > rule #c. Each kobject points to its owning module such that grabbing > a kobject automatically grabs the module. The problem with this > approach is that it requires wide update and makes kobject_get > heavier. Shouldn't getting/putting the module refcount be solely done in kobject.c? Grab the module reference when the kobject is created and release the module reference in kobject_cleanup() after the release function has been called. This doesn't make kobject_get() heavier, and it ensures we don't delete the module until after the last kobject it is supposed to clean up has been released. - 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: [RFD driver-core] Lifetime problems of the current driver model
Hi James, On 3/30/07, James Bottomley <[EMAIL PROTECTED]> wrote: On Fri, 2007-03-30 at 18:43 +0900, Tejun Heo wrote: > Orphaning sysfs nodes on unregistration is a big step in this > direction. With sysfs reference counting out of the picture, > implementing 'disconnect immediate' interface only on a few components > (including request_queue) should suffice for most block device > drivers. I'm not familiar with other drivers but I don't think > they'll be very different. I agree with this statement. The big question in my mind is how do we do it? The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists. It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations. So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on device_add() and relinquish it again (and null out the kobject pointer) on device_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably in device_add() (it doesn't come for free with the device structures). If you want to manage lifetime rules independently you might want to not embed struct device into you subsystems objects but attach them via pointers and use device_create(). Now that we orphan sysfs access upon unregistering device this will severe all ties from driver core to your system once you start teardown of a device and you should be in clear. However there are simpler subsystems (input, serio, etc.) where there is only one core module which provides services to device drivers and handles registration and deregistration. For such substustems it makes sense to embed struct devices and manage lifetime for all components at once. -- Dmitry - 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 disk corruptor paths ?
> There is another ugly here too ata_tf_init sets the device bits but > doesn't set TFLAG_DEVICE. Scarily in fact the qc_reinit path that Argh ignore this part - I meant to delete it as the tf structure is memset to zero. The other case I believe is a genuine bug however - 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 disk corruptor paths ?
Found these by inspection: Command issuing goes via ata_qc_reinit() which sets up the device bits for the command to include the device select bit. If we are using NCQ then the code in ata_build_rw_tf sets bit 6 directly without using |= which clears the device select bit and means any NCQ command will go to the first device regardless. The non NCQ path thankfully doesn't blat the other bits and right now we have no slave devices on NCQ supporting hardware I believe. The same bug is present in the libata patches for HPA, and it pulls the primary HPA blindly in each case. I've not found any others but they may be there and the code is a bit fragile around here. I suspect this is the Macintosh problem. Is there a reason we can't make exec_internel and/or qc_issue BUG() if the passed device and tf device bit disagree ? There is another ugly here too ata_tf_init sets the device bits but doesn't set TFLAG_DEVICE. Scarily in fact the qc_reinit path that produces new qc structures doesn't touch tf->flags at all but leaves them as they were previously (with or without device). Any command issued with a device set but the flag forgotten is going to work *until* we issue a command to one device and forget the flag, after a command to another device, in which case the wrong device will get the command. There are other flags that will cause lunacy to occur as well - LBA48 being an obvious one that will blow up in our faces. Follow the path of a start_stop command - we set the device flag but we assume the newly allocated command flags started clear, not random. We don't clear the LBA48 flag either Alan - 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: [RFD driver-core] Lifetime problems of the current driver model
On Fri, 2007-03-30 at 18:43 +0900, Tejun Heo wrote: > Orphaning sysfs nodes on unregistration is a big step in this > direction. With sysfs reference counting out of the picture, > implementing 'disconnect immediate' interface only on a few components > (including request_queue) should suffice for most block device > drivers. I'm not familiar with other drivers but I don't think > they'll be very different. I agree with this statement. The big question in my mind is how do we do it? The essential problem, and the reason why the lifetime rules are entangled is that fundamentally, sysfs entries are managed by kobjects. The things the device drivers are interested in is struct device, which is usually embedded in driver data structures. Unfortunately, the required kobject is usually embedded in struct device meaning that the relevant driver structure cannot be freed while the sysfs entry still exists. It seems to me that the only way to Orphan the sysfs entries is to separate the kobject and the struct device so their lifetime rules are no longer entangled. Then we can free the driver structure with the embedded struct device while still keeping the necessary kobject around to perform orphaned sysfs operations. So it seems to me that what we need to do is to give struct device its own kref and a pointer to a kobject. Then we can manage the separate lifetimes independently. The device would basically allocate and take a reference to the kobject on device_add() and relinquish it again (and null out the kobject pointer) on device_del(). The complexity here is that we now have to allocate the kobject somewhere else ... probably in device_add() (it doesn't come for free with the device structures). James - 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
[RFD driver-core] Lifetime problems of the current driver model
Hello, all.
This document tries to describe lifetime problems of the current
device driver model primarily from the point view of device drivers
and establish consensus, or at least, start discussion about how to
solve these problems. This is primarily based on my experience with
IDE and SCSI layers and my knowledge on other drivers is limited, so I
might have generalized too aggressively. Feel free to point out.
Table of Contents
1. Why is it this complex?
1-1. Rule #a and #b
1-2. Rule #b and #c
2. How do we solve this?
2-1. Rule #a and #b
2-2. Rule #b and #c
3. How do we get there from here?
4. Some afterthoughts
1. Why is it this complex?
Object lifetime management in device drivers is complex and thus
error-prone under the current driver model. The primary reason is
that device drivers have to match impedances of three different life
time rules.
Rule a. The device itself. Many devices are hot pluggable these days.
Devices come when they come and go when they go.
Rule b. Driver model object lifetime rule. Fundamental data
structures used by device drivers are reference counted and
their lifetime can be arbitrarily extended by userspace sysfs
access.
Rule c. Module life time rule. Most device drivers can be compiled as
modules and thus module busy count should be managed properly.
1-1. Rule #a and #b
Being what it is, the primary concern of a device driver is the
'device' itself. A device driver should create and release objects
representing the devices it manages as they come and go. In one way
or another, this just has to be done, so this rule is pretty obvious
and intuitive to device driver writers.
The current device driver model mandates implementation of rule #b in
device drivers. This makes the object lifetime management complicated
and, more importantly, less intuitive to driver writers. As
implementing such lifetime management in each device driver is too
painful, most device driver midlayers handle this. e.g. SCSI midlayer
handles driver model lifetime rules and presents simplified
register/unregister-immediately interface to all low level SCSI
drivers. IDE tries to do similar and USB and netdev have their own
mechanisms too.
I'm not familiar with other layers, but it took quite a while to get
this right in the SCSI midlayer. The current code works and low level
drivers can do away with most of lifetime management but the added
complexity is saddening to look at.
1-2. Rule #b and #c
On the surface, it seems that we're doing this mostly correctly but
subtle bugs caused by interaction between driver model lifetime and
module lifetime rules aren't difficult to find.
This happens because the two lifetime rules are closely related but
the relation is not properly represented in device driver model
itself. A module cannot be unloaded unless all device driver objects
it hosts are released, but the only way to guarantee that is to always
grab module reference whenever grabbing relevant kobject reference.
This is ugly to code and too easy to get wrong. Here are two such
examples.
Example 1. sysfs_schedule_callback() not grabbing the owning module
This function is recently added to be used by suicidal sysfs nodes
such that they don't deadlock when trying to unregister themselves.
+#include
static void sysfs_schedule_callback_work(struct work_struct *work)
{
struct sysfs_schedule_callback_struct *ss = container_of(work,
struct sysfs_schedule_callback_struct, work);
+ msleep(100);
(ss->func)(ss->data);
kobject_put(ss->kobj);
kfree(ss);
}
int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),
void *data)
{
struct sysfs_schedule_callback_struct *ss;
ss = kmalloc(sizeof(*ss), GFP_KERNEL);
if (!ss)
return -ENOMEM;
kobject_get(kobj);
ss->kobj = kobj;
ss->func = func;
ss->data = data;
INIT_WORK(&ss->work, sysfs_schedule_callback_work);
schedule_work(&ss->work);
return 0;
}
Two lines starting with '+' are inserted to make the problem
reliably reproducible. With the above changes,
# insmod drivers/scsi/scsi_mod.ko; insmod drivers/scsi/sd_mod.ko; insmod
drivers/ata/libata.ko; insmod drivers/ata/ahci.ko
# echo 1 > /sys/block/sda/device/delete; rmmod ahci; rmmod libata; rmmod
sd_mod; rmmod scsi_mod
It's assumed that ahci detects /dev/sda. The above command sequence
causes the following oops.
BUG: unable to handle kernel paging request at virtual address e0984020
[--snip--]
EIP is at 0xe0984020
[--snip--]
[] run_workqueue+0x92/0x140
[] worker_thread+0x137/0x160
[] kthread+0xa3/0xd0
[] kernel_thread_helper+0x7/0x10
The problem here is that kobjec_get() in sysfs_schedule_callback()
doesn't grab the module backing the kobject it's grabbing. By the
time (ss->func)(ss->kobj) runs, scsi_mod is already gone.
Example 2. sys
