Fix interface autodetection in legacy IDE driver (trial #2)

PATCH In review Luiz Carlos Ramos 3 participants 14 messages Oct 12, 2016 – Feb 21, 2017 libata

lore

Share & tools

lore

Luiz Carlos Ramos

author

2016-10-12 01:17 UTC #0 lore raw

PATCH 1/1 Fix interface autodetection in legacy IDE driver (trial #2)

Hello,

This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).

It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).

Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.

So, again IMHO, it is worth be fixed.

This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.

Best regards,

Signed-off-by Luiz Carlos Ramos <[email protected]>

---
 drivers/ide/ide-generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/ide/ide-generic.c +2 -2

index 54d7c4685d23aa5e62ce606e7b994a57bb54b08a..419818a39c270d3ad219e8f7b5df56a9aea3d640 100644
@@ -96,10 +96,10 @@ static int __init ide_generic_init(void)
 		printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" "
 		     "module parameter for probing all legacy ISA IDE ports\n");
 
-		if (primary == 0)
+		if (primary)
 			probe_mask |= 0x1;
 
-		if (secondary == 0)
+		if (secondary)
 			probe_mask |= 0x2;
 	} else
 		printk(KERN_INFO DRV_NAME ": enforcing probing of I/O ports "

Signature · 1 line

2.8.2

David Miller

2016-12-26 16:47 UTC #1 lore raw

From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 11 Oct 2016 22:12:45 -0300

Quoted 13 lines

This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.

Quoted 7 lines

This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>

This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.

Applied and queued up for -stable, th anks.

Bartlomiej Zolnierkiewicz

2016-12-27 10:08 UTC #2 lore raw

Hi,

Quoted 20 lines · David Miller

From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 11 Oct 2016 22:12:45 -0300
Quoted 17 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.

Quoted 7 lines

Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.

Please always cc: the original commit author.

Quoted 1 line

Applied and queued up for -stable, th anks.

For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:

https://patchwork.ozlabs.org/patch/680975/

The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Bartlomiej Zolnierkiewicz

2016-12-27 16:06 UTC #3 lore raw

Quoted 39 lines · Bartlomiej Zolnierkiewicz

Hi,
Quoted 20 lines · David Miller
From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 11 Oct 2016 22:12:45 -0300
Quoted 17 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:

Quoted 7 lines

https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).

Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.

Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

David Miller

2016-12-27 16:41 UTC #4 lore raw

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 27 Dec 2016 17:06:19 +0100

Quoted 48 lines · Bartlomiej Zolnierkiewicz

Quoted 39 lines
Hi,
Quoted 20 lines · David Miller
From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 11 Oct 2016 22:12:45 -0300
Quoted 17 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:
Quoted 7 lines
https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).

Quoted 8 lines

Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.
Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?

The original code before the patch in question probed the interfaces unconditionally, probe_mask was a static int set to "0x03".

Commit 20df429dd6671804999493baf2952f82582869fa changed the default behavior, as well as adding a new module parameter whose behavior makes no sense at all. Inverted bit logic? Give me a break.

Sorry, no, the fix is correct and I'm pushing it to Linus.

Bartlomiej Zolnierkiewicz

2016-12-27 17:26 UTC #5 lore raw

Hi,

Quoted 60 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 27 Dec 2016 17:06:19 +0100
Quoted 48 lines · Bartlomiej Zolnierkiewicz
Quoted 39 lines
Hi,
Quoted 20 lines · David Miller
From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 11 Oct 2016 22:12:45 -0300
Quoted 17 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:
Quoted 7 lines
https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).
Quoted 8 lines
Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.
Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?

Quoted 8 lines

The original code before the patch in question probed the interfaces unconditionally, probe_mask was a static int set to "0x03".
Commit 20df429dd6671804999493baf2952f82582869fa changed the default behavior, as well as adding a new module parameter whose behavior makes no sense at all. Inverted bit logic? Give me a break.
Sorry, no, the fix is correct and I'm pushing it to Linus.

The "fix" is not correct and is not needed. 99% of users of ide-generic used it by mistake and should have used the designated host drivers for their hardware or PCI IDE generic host driver (not ide-generic one).

Alan Cox did the work on fixing this for his pata_legacy libata host driver and later Borislav back-ported needed changes to ide-generic host driver in commit 20df429dd6671804999493baf2952f82582869fa (in *2008*).

Also the "fix" is not a revert but a new patch which introduces a real regression described by me in the previous mail.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Luiz Carlos Ramos

author

2016-12-28 00:38 UTC #6 lore raw

Hi,

I really don't know if my opinions will be well received, but here we go.

Please apologize me if you feel me rude, but I'm just trying to explain the problem the way I'm seeing it, and also to understand what you are saying the best I can.

Quoted 50 lines · Bartlomiej Zolnierkiewicz

Hi,
Quoted 46 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 27 Dec 2016 17:06:19 +0100
Quoted 43 lines · Bartlomiej Zolnierkiewicz
Quoted 42 lines
Hi,
Quoted 20 lines · David Miller
From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 11 Oct 2016 22:12:45 -0300
Quoted 17 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:
https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in

I think all of you have checked the code, but I will try to describe what I'm seeing.

The routine ide_generic_check_pci_legacy_iobases() returns *primary equal to 1 if some PCI IDE resource is found at 0x1f0, and *secondary equal to 1 if some PCI IDE resource is found at 0x1e0. The same routine doesn't change neither *primary not *secondary if nothing is found. The only caller - ide_generic_init() - initializes both to zero before the call.

So, ide_generic_init() should test *primary for 1 in the case of an existing IDE primary resource, and *secondary for 1 in the case of a secondary IDE resource.

Unpatched code checks both for zero in order to set the proper bits in probe_mask, and IMHO this is reversed logic.

There are two ways of fixing this. One is to test *primary and *secondary for "not zero" in ide_generic_init(), as proposed. The other way is to change ide_generic_check_pci_legacy_iobases() in order to return *primary or *secondary with zero in case of success when searching for PCI resources - along with setting them to "not zero" before checking.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).

Please elaborate more on this; I really haven't understood. It seems that you're telling about PCI host driver code, which I haven't studied. I haven't caught the whole picture.

In my case, there are two IDE interfaces, which are at the common addresses 0x1f0 and 0x1e0 (PCI also tells me that), and the chipset is a CS5536. The previous kernel used in the system (2.6.16) worked with ide-generic, and the one I'm testing now (3.18.x) doesn't bring any of the IDE interfaces up.

You're right in the sense I haven't compiled the CS5536 legacy host driver code; so, I can't tell about how it works.

Of course there are many reasons to use the correct PCI IDE host driver, but at the end, it's up to the user to choose which one he/she will use, and ideally any of them should work.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.

Again, please elaborate more on this.

If there are no PCI IDE devices, well, there are no PCI IDE devices. Nothing is supposed to be found. Anyway, it seems they will be searched for, in ide_generic_check_pci_legacy_iobases(). Or I missed something important.

Quoted 11 lines

Quoted 11 lines
Quoted 3 lines
Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?
The original code before the patch in question probed the interfaces unconditionally, probe_mask was a static int set to "0x03".
Commit 20df429dd6671804999493baf2952f82582869fa changed the default behavior, as well as adding a new module parameter whose behavior makes no sense at all. Inverted bit logic? Give me a break.

Quoted 5 lines

Quoted 1 line
Sorry, no, the fix is correct and I'm pushing it to Linus.
The "fix" is not correct and is not needed. 99% of users of ide-generic used it by mistake and should have used the designated host drivers for their hardware or PCI IDE generic host driver (not ide-generic one).

My system didn't work without this patch. I agree I could use the designated host driver, but it seems a little strong to say one _should_ use it.

A newer version of the system uses libata, and yes, it works.

Quoted 8 lines

Alan Cox did the work on fixing this for his pata_legacy libata host driver and later Borislav back-ported needed changes to ide-generic host driver in commit 20df429dd6671804999493baf2952f82582869fa (in *2008*).
Also the "fix" is not a revert but a new patch which introduces a real regression described by me in the previous mail.

As said above, it is a regression considering the versions from 2006. I agree that for someone who used the drivers from 2009 and after, it _may_ be a regression if she uses to bring all up with probe_mask=0.

IMHO, it's the case where a bug later becomes a feature.

Quoted 6 lines

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Best regards,

Luiz Carlos Ramos
São Paulo - Brazil

David Miller

2016-12-28 00:38 UTC #7 lore raw

From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 27 Dec 2016 22:33:35 -0200

Quoted 6 lines

So, ide_generic_init() should test *primary for 1 in the case of an existing IDE primary resource, and *secondary for 1 in the case of a secondary IDE resource.
Unpatched code checks both for zero in order to set the proper bits in probe_mask, and IMHO this is reversed logic.

Right, and I can't see how this is intentional at all.

Bartlomiej Zolnierkiewicz

2016-12-28 11:16 UTC #8 lore raw

Hi,

Quoted 11 lines · David Miller

From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 27 Dec 2016 22:33:35 -0200
Quoted 6 lines
So, ide_generic_init() should test *primary for 1 in the case of an existing IDE primary resource, and *secondary for 1 in the case of a secondary IDE resource.
Unpatched code checks both for zero in order to set the proper bits in probe_mask, and IMHO this is reversed logic.
Right, and I can't see how this is intentional at all.

If PCI IDE resource is found we shouldn't do the probe automatically as ide-generic is not the proper driver to run the hardware. IOW we only want to do automatic probe if no PCI IDE resources are found (if primary/secondary == 0). In such case we are most likely running on pre-PCI system and should be probing for legacy ISA IDE ports. Please note they are using the same I/O resources as PCI IDE ones (!).

[ Please see the reply to Luiz for the full explanation. ]

I know that you're busy and don't have time for in-depth analysis of non-obvious IDE patches. I would like to help and (co-)maintain the IDE subsystem (deep maintenance mode policy would of course be preserved).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Bartlomiej Zolnierkiewicz

2016-12-28 11:20 UTC #9 lore raw

In reply to Luiz Carlos Ramos

Hi,

Quoted 70 lines · Luiz Carlos Ramos

Hi,
I really don't know if my opinions will be well received, but here we go.
Please apologize me if you feel me rude, but I'm just trying to explain the problem the way I'm seeing it, and also to understand what you are saying the best I can.
Quoted 50 lines · Bartlomiej Zolnierkiewicz
Hi,
Quoted 46 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 27 Dec 2016 17:06:19 +0100
Quoted 43 lines · Bartlomiej Zolnierkiewicz
Quoted 42 lines
Hi,
Quoted 20 lines · David Miller
From: Luiz Carlos Ramos <[email protected]>
Date: Tue, 11 Oct 2016 22:12:45 -0300
Quoted 17 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:
https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in
I think all of you have checked the code, but I will try to describe what I'm seeing.
The routine ide_generic_check_pci_legacy_iobases() returns *primary equal to 1 if some PCI IDE resource is found at 0x1f0, and *secondary equal to 1 if some PCI IDE resource is found at 0x1e0. The same routine doesn't change neither *primary not *secondary if nothing is found. The only caller - ide_generic_init() - initializes both to zero before the call.

Quoted 6 lines

So, ide_generic_init() should test *primary for 1 in the case of an existing IDE primary resource, and *secondary for 1 in the case of a secondary IDE resource.
Unpatched code checks both for zero in order to set the proper bits in probe_mask, and IMHO this is reversed logic.

Unpatched code is correct.

Quoted 6 lines

There are two ways of fixing this. One is to test *primary and *secondary for "not zero" in ide_generic_init(), as proposed. The other way is to change ide_generic_check_pci_legacy_iobases() in order to return *primary or *secondary with zero in case of success when searching for PCI resources - along with setting them to "not zero" before checking.

No need to change anything. Original code is correct.

Quoted 11 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).
Please elaborate more on this; I really haven't understood. It seems that you're telling about PCI host driver code, which I haven't studied. I haven't caught the whole picture.

Quoted 8 lines

In my case, there are two IDE interfaces, which are at the common addresses 0x1f0 and 0x1e0 (PCI also tells me that), and the chipset is a CS5536. The previous kernel used in the system (2.6.16) worked with ide-generic, and the one I'm testing now (3.18.x) doesn't bring any of the IDE interfaces up.
You're right in the sense I haven't compiled the CS5536 legacy host driver code; so, I can't tell about how it works.

You should be using CS5336 PCI IDE host driver (drivers/ide/cs5536.c enabled by CONFIG_BLK_DEV_CS5536 config option) for optimal performance and proper operations.

CS5536 PCI IDE host driver was added in kernel v2.6.29.

Quoted 3 lines

Of course there are many reasons to use the correct PCI IDE host driver, but at the end, it's up to the user to choose which one he/she will use, and ideally any of them should work.

It happens to "work" on your system because CS5536 is relatively simple and non-buggy PCI IDE chipset.

In case of other PCI IDE chipsets you may not only be missing performance or some functionality - on some more quirky chipsets you may be also affecting data integrity (!).

Anyway you still can use ide-generic by using kernel parameters (just add "ide_generic.probe_mask=0x03" to your kernel command line).

I agree that the ide-generic could be more verbose and print more information in case of skipping the probe (patches are welcomed).

Quoted 6 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.

Quoted 6 lines

Again, please elaborate more on this.
If there are no PCI IDE devices, well, there are no PCI IDE devices. Nothing is supposed to be found. Anyway, it seems they will be searched for, in ide_generic_check_pci_legacy_iobases(). Or I missed something important.

In case there are no PCI IDE devices we most likely are running on pre-PCI system and should be probing for legacy ISA IDE ports (please note they are are using the same I/O resources),

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 3 lines
Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?
The original code before the patch in question probed the interfaces unconditionally, probe_mask was a static int set to "0x03".
Commit 20df429dd6671804999493baf2952f82582869fa changed the default behavior, as well as adding a new module parameter whose behavior makes no sense at all. Inverted bit logic? Give me a break.
Sorry, no, the fix is correct and I'm pushing it to Linus.

Quoted 7 lines

Quoted 3 lines
The "fix" is not correct and is not needed. 99% of users of ide-generic used it by mistake and should have used the designated host drivers for their hardware or PCI IDE generic host driver (not ide-generic one).
My system didn't work without this patch. I agree I could use the designated host driver, but it seems a little strong to say one _should_ use it.

Sorry but one should really use the designated drivers and we don't want to see any bugreports from using ide-generic on hardware that have designated drivers.

Quoted 1 line

A newer version of the system uses libata, and yes, it works.

In libata there is the same policy as in IDE - pata_legacy host driver (libata's equivalent of ide-generic host driver) was the first one using the new policy, it was later back-ported to ide-generic.

Quoted 11 lines

Quoted 7 lines
Alan Cox did the work on fixing this for his pata_legacy libata host driver and later Borislav back-ported needed changes to ide-generic host driver in commit 20df429dd6671804999493baf2952f82582869fa (in *2008*).
Also the "fix" is not a revert but a new patch which introduces a real regression described by me in the previous mail.
As said above, it is a regression considering the versions from 2006. I agree that for someone who used the drivers from 2009 and after, it _may_ be a regression if she uses to bring all up with probe_mask=0.

The regression I was talking about is in the proposed patch.

By simply reversing the logic ISA IDE ports will no longer be probed automatically on non-PCI systems.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Quoted 13 lines

IMHO, it's the case where a bug later becomes a feature.
Quoted 6 lines
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
Best regards,
Luiz Carlos Ramos
São Paulo - Brazil

Luiz Carlos Ramos

author

2016-12-30 00:58 UTC #10 lore raw

Hi,

I think I could catch the whole picture after your explanation. I'll try to summarize all the points discussed below in the body.

My suggestion now is to drop the current (proposed) patch and substitute it for a pure "documentational" patch, as it seems for me that a programmer new to that subsystem - as the case of myself - tends to find ide-generic.c buggy, if analyzed out of the whole context. Some more inline documentation may help all of us IMHO.

Of course improving the printed messages - as suggested - can help as well.

I'd like to hear from David (cc'ed) about this suggestion. What would you David suggest?

Anyway, there is a "break of the contract" with the old patch which introduced the search for the IDE interfaces. Before, it was not necessary to specify probe_mask=0x03 or like to make ide-generic find something in I/O addresses 0x1f0 and 0x170 (I wrongly wrote 0x1e0 in the former emails; please apologize me for that mistake). But this contract break seems to be justifiable on the grounds that - by a design decision - users "are encouraged" to use the designated drivers as a basic axiom. I suppose (or I hope) this was discussed at that time and this decision was taken consciously.

Quoted 53 lines · Bartlomiej Zolnierkiewicz

Quoted 46 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Quoted 17 lines
Quoted 17 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:
https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in
I think all of you have checked the code, but I will try to describe what I'm seeing.
The routine ide_generic_check_pci_legacy_iobases() returns *primary equal to 1 if some PCI IDE resource is found at 0x1f0, and *secondary equal to 1 if some PCI IDE resource is found at 0x1e0. The same routine doesn't change neither *primary not *secondary if nothing is found. The only caller - ide_generic_init() - initializes both to zero before the call.
Quoted 6 lines
So, ide_generic_init() should test *primary for 1 in the case of an existing IDE primary resource, and *secondary for 1 in the case of a secondary IDE resource.
Unpatched code checks both for zero in order to set the proper bits in probe_mask, and IMHO this is reversed logic.

Quoted 9 lines

Unpatched code is correct.
If PCI IDE resource is found we shouldn't do the probe automatically as ide-generic is not the proper driver to run the hardware. IOW we only want to do automatic probe if no PCI IDE resources are found (if primary/secondary == 0). In such case we are most likely running on pre-PCI system and should be probing for legacy ISA IDE ports. Please note they are are using the same I/O resources as PCI IDE ones (!).

Understood. ide-generic should not use devices which have a PCI interface, unless the user tells it to do it (using probe_mask!=0).

The basic assumption - as stated I think in your first reply - is that by design, users should use the driver more suitable for the chipset, if it has a PCI interface, and not ide-generic.

I think this assumption should be highlighted as an axiom, or a design decision. Otherwise, one could argue in favor of enabling ide-generic as a first option for handling the IDE interface, be that PCI or IDE.

Also is supposed that this decision is not subject to discussion (now).

Quoted 17 lines

Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).
Please elaborate more on this; I really haven't understood. It seems that you're telling about PCI host driver code, which I haven't studied. I haven't caught the whole picture.
In my case, there are two IDE interfaces, which are at the common addresses 0x1f0 and 0x1e0 (PCI also tells me that), and the chipset is a CS5536. The previous kernel used in the system (2.6.16) worked with ide-generic, and the one I'm testing now (3.18.x) doesn't bring any of the IDE interfaces up.

Quoted 8 lines

Quoted 2 lines
You're right in the sense I haven't compiled the CS5536 legacy host driver code; so, I can't tell about how it works.
You should be using CS5336 PCI IDE host driver (drivers/ide/cs5536.c enabled by CONFIG_BLK_DEV_CS5536 config option) for optimal performance and proper operations.
CS5536 PCI IDE host driver was added in kernel v2.6.29.

This can be one reason to not have CS5536 driver used in the first version of the system (kernel 2.6.16).

Quoted 8 lines

Quoted 4 lines
Of course there are many reasons to use the correct PCI IDE host driver, but at the end, it's up to the user to choose which one he/she will use, and ideally any of them should work.
It happens to "work" on your system because CS5536 is relatively simple and non-buggy PCI IDE chipset.

Quoted 7 lines

In case of other PCI IDE chipsets you may not only be missing performance or some functionality - on some more quirky chipsets you may be also affecting data integrity (!).
Anyway you still can use ide-generic by using kernel parameters (just add "ide_generic.probe_mask=0x03" to your kernel command line).

As said before, this is a contract break between versions from, let's say, 2006 and today's.

Anyway, there's no way to avoid it, it we take the assumption above (designated drivers prior to ide-generic) as an axiom.

Quoted 3 lines

I agree that the ide-generic could be more verbose and print more information in case of skipping the probe (patches are welcomed).

I think this is a good idea, but more important IMHO is to document properly in the code all this discussion. If I have some spare time (not sure about this), I'll try to submit patches (better to have two) to address both "issues".

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.
Again, please elaborate more on this.

Quoted 9 lines

Quoted 4 lines
If there are no PCI IDE devices, well, there are no PCI IDE devices. Nothing is supposed to be found. Anyway, it seems they will be searched for, in ide_generic_check_pci_legacy_iobases(). Or I missed something important.
In case there are no PCI IDE devices we most likely are running on pre-PCI system and should be probing for legacy ISA IDE ports (please note they are are using the same I/O resources),

Ok, that is, ide-generic is tailored to pure ISA devices, and _can_ work in some PCI devices, although not guaranteed, on user request (probe_mask!=0).

Quoted 17 lines

Quoted 17 lines
Quoted 16 lines
Quoted 4 lines
Quoted 3 lines
Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?
Quoted 8 lines
The original code before the patch in question probed the interfaces unconditionally, probe_mask was a static int set to "0x03".
Commit 20df429dd6671804999493baf2952f82582869fa changed the default behavior, as well as adding a new module parameter whose behavior makes no sense at all. Inverted bit logic? Give me a break.
Sorry, no, the fix is correct and I'm pushing it to Linus.
The "fix" is not correct and is not needed. 99% of users of ide-generic used it by mistake and should have used the designated host drivers for their hardware or PCI IDE generic host driver (not ide-generic one).

Quoted 8 lines

Quoted 3 lines
My system didn't work without this patch. I agree I could use the designated host driver, but it seems a little strong to say one _should_ use it.
Sorry but one should really use the designated drivers and we don't want to see any bugreports from using ide-generic on hardware that have designated drivers.

As said above, this is a kind of axiom, and I understand this is a conscious decision made somewhere in the past.

Avoiding extra bug reports is a smart decision, anyway.

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
Alan Cox did the work on fixing this for his pata_legacy libata host driver and later Borislav back-ported needed changes to ide-generic host driver in commit 20df429dd6671804999493baf2952f82582869fa (in *2008*).
Also the "fix" is not a revert but a new patch which introduces a real regression described by me in the previous mail.

Quoted 9 lines

Quoted 3 lines
As said above, it is a regression considering the versions from 2006. I agree that for someone who used the drivers from 2009 and after, it _may_ be a regression if she uses to bring all up with probe_mask=0.
The regression I was talking about is in the proposed patch.
By simply reversing the logic ISA IDE ports will no longer be probed automatically on non-PCI systems.

Understood. It makes sense.

Bartlomiej Zolnierkiewicz

2016-12-30 16:05 UTC #11 lore raw

Hi,

[ Your mails don't make it into linux-ide mailing list for some reason
(the reason why I have not seen your patch before Dave applied it). ]

Quoted 10 lines · Luiz Carlos Ramos

Hi,
I think I could catch the whole picture after your explanation. I'll try to summarize all the points discussed below in the body.
My suggestion now is to drop the current (proposed) patch and substitute it for a pure "documentational" patch, as it seems for me that a programmer new to that subsystem - as the case of myself - tends to find ide-generic.c buggy, if analyzed out of the whole context. Some more inline documentation may help all of us IMHO.

I agree. BTW ide-generic is a bad name (my fault), it should have been ide-isa or (better) ide-legacy.

Quoted 5 lines

Of course improving the printed messages - as suggested - can help as well.
I'd like to hear from David (cc'ed) about this suggestion. What would you David suggest?

Quoted 8 lines

Anyway, there is a "break of the contract" with the old patch which introduced the search for the IDE interfaces. Before, it was not necessary to specify probe_mask=0x03 or like to make ide-generic find something in I/O addresses 0x1f0 and 0x170 (I wrongly wrote 0x1e0 in the former emails; please apologize me for that mistake). But this contract break seems to be justifiable on the grounds that - by a design decision - users "are encouraged" to use the designated drivers as a basic axiom. I suppose (or I hope) this was discussed at that time and this decision was taken consciously.

Yes, it was a conscious decision.

Quoted 72 lines · Bartlomiej Zolnierkiewicz

Quoted 54 lines
Quoted 47 lines
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Quoted 18 lines
Quoted 18 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:
https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in
I think all of you have checked the code, but I will try to describe what I'm seeing.
The routine ide_generic_check_pci_legacy_iobases() returns *primary equal to 1 if some PCI IDE resource is found at 0x1f0, and *secondary equal to 1 if some PCI IDE resource is found at 0x1e0. The same routine doesn't change neither *primary not *secondary if nothing is found. The only caller - ide_generic_init() - initializes both to zero before the call.
Quoted 6 lines
So, ide_generic_init() should test *primary for 1 in the case of an existing IDE primary resource, and *secondary for 1 in the case of a secondary IDE resource.
Unpatched code checks both for zero in order to set the proper bits in probe_mask, and IMHO this is reversed logic.
Quoted 9 lines
Unpatched code is correct.
If PCI IDE resource is found we shouldn't do the probe automatically as ide-generic is not the proper driver to run the hardware. IOW we only want to do automatic probe if no PCI IDE resources are found (if primary/secondary == 0). In such case we are most likely running on pre-PCI system and should be probing for legacy ISA IDE ports. Please note they are are using the same I/O resources as PCI IDE ones (!).
Understood. ide-generic should not use devices which have a PCI interface, unless the user tells it to do it (using probe_mask!=0).
The basic assumption - as stated I think in your first reply - is that by design, users should use the driver more suitable for the chipset, if it has a PCI interface, and not ide-generic.

Quoted 5 lines

I think this assumption should be highlighted as an axiom, or a design decision. Otherwise, one could argue in favor of enabling ide-generic as a first option for handling the IDE interface, be that PCI or IDE.
Also is supposed that this decision is not subject to discussion (now).

You are correct, also IDE subsystem is now in deep maintenance mode.

Quoted 20 lines

Quoted 20 lines
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).
Please elaborate more on this; I really haven't understood. It seems that you're telling about PCI host driver code, which I haven't studied. I haven't caught the whole picture.
In my case, there are two IDE interfaces, which are at the common addresses 0x1f0 and 0x1e0 (PCI also tells me that), and the chipset is a CS5536. The previous kernel used in the system (2.6.16) worked with ide-generic, and the one I'm testing now (3.18.x) doesn't bring any of the IDE interfaces up.
You're right in the sense I haven't compiled the CS5536 legacy host driver code; so, I can't tell about how it works.

Quoted 8 lines

Quoted 5 lines
You should be using CS5336 PCI IDE host driver (drivers/ide/cs5536.c enabled by CONFIG_BLK_DEV_CS5536 config option) for optimal performance and proper operations.
CS5536 PCI IDE host driver was added in kernel v2.6.29.
This can be one reason to not have CS5536 driver used in the first version of the system (kernel 2.6.16).

Please note that CS5536 is supported also in amd74xx.c host driver (for historical reasons) and this support was merged in 2005 in commit 7fab773de16ccaeb249acdc6e956a9759c68225d (kernel v2.6.15).

Also even before that ide-pci-generic.c driver has been available which is still more suitable than ide-generic.c one for CS5536 (it needs to be forced with "ide_pci_generic.all_generic_ide" kernel command line parameter).

Quoted 21 lines

Quoted 7 lines
Quoted 3 lines
Of course there are many reasons to use the correct PCI IDE host driver, but at the end, it's up to the user to choose which one he/she will use, and ideally any of them should work.
It happens to "work" on your system because CS5536 is relatively simple and non-buggy PCI IDE chipset.
Quoted 7 lines
In case of other PCI IDE chipsets you may not only be missing performance or some functionality - on some more quirky chipsets you may be also affecting data integrity (!).
Anyway you still can use ide-generic by using kernel parameters (just add "ide_generic.probe_mask=0x03" to your kernel command line).
As said before, this is a contract break between versions from, let's say, 2006 and today's.
Anyway, there's no way to avoid it, it we take the assumption above (designated drivers prior to ide-generic) as an axiom.

Quoted 8 lines

Quoted 3 lines
I agree that the ide-generic could be more verbose and print more information in case of skipping the probe (patches are welcomed).
I think this is a good idea, but more important IMHO is to document properly in the code all this discussion. If I have some spare time (not sure about this), I'll try to submit patches (better to have two) to address both "issues".

Thank you, it would be good to have this covered.

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.
Again, please elaborate more on this.
If there are no PCI IDE devices, well, there are no PCI IDE devices. Nothing is supposed to be found. Anyway, it seems they will be searched for, in ide_generic_check_pci_legacy_iobases(). Or I missed something important.

Quoted 8 lines

Quoted 4 lines
In case there are no PCI IDE devices we most likely are running on pre-PCI system and should be probing for legacy ISA IDE ports (please note they are are using the same I/O resources),
Ok, that is, ide-generic is tailored to pure ISA devices, and _can_ work in some PCI devices, although not guaranteed, on user request (probe_mask!=0).

Yes.

Quoted 43 lines

Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 4 lines
Quoted 3 lines
Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?
Quoted 8 lines
The original code before the patch in question probed the interfaces unconditionally, probe_mask was a static int set to "0x03".
Commit 20df429dd6671804999493baf2952f82582869fa changed the default behavior, as well as adding a new module parameter whose behavior makes no sense at all. Inverted bit logic? Give me a break.
Sorry, no, the fix is correct and I'm pushing it to Linus.
The "fix" is not correct and is not needed. 99% of users of ide-generic used it by mistake and should have used the designated host drivers for their hardware or PCI IDE generic host driver (not ide-generic one).
Quoted 8 lines
Quoted 3 lines
My system didn't work without this patch. I agree I could use the designated host driver, but it seems a little strong to say one _should_ use it.
Sorry but one should really use the designated drivers and we don't want to see any bugreports from using ide-generic on hardware that have designated drivers.
As said above, this is a kind of axiom, and I understand this is a conscious decision made somewhere in the past.
Avoiding extra bug reports is a smart decision, anyway.
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
Alan Cox did the work on fixing this for his pata_legacy libata host driver and later Borislav back-ported needed changes to ide-generic host driver in commit 20df429dd6671804999493baf2952f82582869fa (in *2008*).
Also the "fix" is not a revert but a new patch which introduces a real regression described by me in the previous mail.
As said above, it is a regression considering the versions from 2006. I agree that for someone who used the drivers from 2009 and after, it _may_ be a regression if she uses to bring all up with probe_mask=0.

Quoted 7 lines

Quoted 5 lines
The regression I was talking about is in the proposed patch.
By simply reversing the logic ISA IDE ports will no longer be probed automatically on non-PCI systems.
Understood. It makes sense.

Thank you for understanding.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

David Miller

2017-01-09 20:25 UTC #12 lore raw

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Fri, 30 Dec 2016 17:05:51 +0100

Quoted 1 line

Yes, it was a conscious decision.

I understand things now, I'll revert this change, thanks everyone.

Luiz Carlos Ramos

author

2017-02-21 14:22 UTC #13 lore raw

In reply to Bartlomiej Zolnierkiewicz

Hi, Bartlomiej, David and list!

Quoted 180 lines · Bartlomiej Zolnierkiewicz

Hi,
[ Your mails don't make it into linux-ide mailing list for some reason
(the reason why I have not seen your patch before Dave applied it). ]
Quoted 10 lines · Luiz Carlos Ramos
Hi,
I think I could catch the whole picture after your explanation. I'll try to summarize all the points discussed below in the body.
My suggestion now is to drop the current (proposed) patch and substitute it for a pure "documentational" patch, as it seems for me that a programmer new to that subsystem - as the case of myself - tends to find ide-generic.c buggy, if analyzed out of the whole context. Some more inline documentation may help all of us IMHO.
I agree. BTW ide-generic is a bad name (my fault), it should have been ide-isa or (better) ide-legacy.
Quoted 5 lines
Of course improving the printed messages - as suggested - can help as well.
I'd like to hear from David (cc'ed) about this suggestion. What would you David suggest?
Quoted 8 lines
Anyway, there is a "break of the contract" with the old patch which introduced the search for the IDE interfaces. Before, it was not necessary to specify probe_mask=0x03 or like to make ide-generic find something in I/O addresses 0x1f0 and 0x170 (I wrongly wrote 0x1e0 in the former emails; please apologize me for that mistake). But this contract break seems to be justifiable on the grounds that - by a design decision - users "are encouraged" to use the designated drivers as a basic axiom. I suppose (or I hope) this was discussed at that time and this decision was taken consciously.
Yes, it was a conscious decision.
Quoted 72 lines · Bartlomiej Zolnierkiewicz
Quoted 54 lines
Quoted 47 lines
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
Quoted 18 lines
Quoted 18 lines
This humble patch was sent one or two months before, and had no actions, except for a colleague reply which friendly pointed out some formatting problems (which were solved in a second message).
It relates to an old code, the legacy IDE driver, but the bug it addresses is real. The code, although rarely used, is still there to be compiled if one chooses to do so (like me).
Also, the fix has a very low risk of present collateral effects IMHO. It is already compiled and tested in some embedded machines.
So, again IMHO, it is worth be fixed.
This email is a second trial with it. I hope it can help the one or two guys out there which are still running the legacy IDE driver and haven't noticed the former email.
Quoted 7 lines
Quoted 3 lines
Best regards,
Signed-off-by Luiz Carlos Ramos <[email protected]>
This bug was introduced by commit 20df429dd6671804999493baf2952f82582869fa ("ide-generic: handle probing of legacy io-ports v5") which seems poorly tested.
Please always cc: the original commit author.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Applied and queued up for -stable, th anks.
For some reason I've never got the discussed patch from linux-ide ML though I now have found in the patchwork:
https://patchwork.ozlabs.org/patch/680975/
The patch is incorrect. If you have PCI IDE devices (like in
I think all of you have checked the code, but I will try to describe what I'm seeing.
The routine ide_generic_check_pci_legacy_iobases() returns *primary equal to 1 if some PCI IDE resource is found at 0x1f0, and *secondary equal to 1 if some PCI IDE resource is found at 0x1e0. The same routine doesn't change neither *primary not *secondary if nothing is found. The only caller - ide_generic_init() - initializes both to zero before the call.
Quoted 6 lines
So, ide_generic_init() should test *primary for 1 in the case of an existing IDE primary resource, and *secondary for 1 in the case of a secondary IDE resource.
Unpatched code checks both for zero in order to set the proper bits in probe_mask, and IMHO this is reversed logic.
Quoted 9 lines
Unpatched code is correct.
If PCI IDE resource is found we shouldn't do the probe automatically as ide-generic is not the proper driver to run the hardware. IOW we only want to do automatic probe if no PCI IDE resources are found (if primary/secondary == 0). In such case we are most likely running on pre-PCI system and should be probing for legacy ISA IDE ports. Please note they are are using the same I/O resources as PCI IDE ones (!).
Understood. ide-generic should not use devices which have a PCI interface, unless the user tells it to do it (using probe_mask!=0).
The basic assumption - as stated I think in your first reply - is that by design, users should use the driver more suitable for the chipset, if it has a PCI interface, and not ide-generic.
Quoted 5 lines
I think this assumption should be highlighted as an axiom, or a design decision. Otherwise, one could argue in favor of enabling ide-generic as a first option for handling the IDE interface, be that PCI or IDE.
Also is supposed that this decision is not subject to discussion (now).
You are correct, also IDE subsystem is now in deep maintenance mode.
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
The patch is incorrect. If you have PCI IDE devices (like in the case described in the situation being "fixed" by the patch) you should use the correct PCI IDE host driver for proper operation and not ide-generic host driver (the latter still can be used by using kernel parameters).
Please elaborate more on this; I really haven't understood. It seems that you're telling about PCI host driver code, which I haven't studied. I haven't caught the whole picture.
In my case, there are two IDE interfaces, which are at the common addresses 0x1f0 and 0x1e0 (PCI also tells me that), and the chipset is a CS5536. The previous kernel used in the system (2.6.16) worked with ide-generic, and the one I'm testing now (3.18.x) doesn't bring any of the IDE interfaces up.
You're right in the sense I haven't compiled the CS5536 legacy host driver code; so, I can't tell about how it works.
Quoted 8 lines
Quoted 5 lines
You should be using CS5336 PCI IDE host driver (drivers/ide/cs5536.c enabled by CONFIG_BLK_DEV_CS5536 config option) for optimal performance and proper operations.
CS5536 PCI IDE host driver was added in kernel v2.6.29.
This can be one reason to not have CS5536 driver used in the first version of the system (kernel 2.6.16).
Please note that CS5536 is supported also in amd74xx.c host driver (for historical reasons) and this support was merged in 2005 in commit 7fab773de16ccaeb249acdc6e956a9759c68225d (kernel v2.6.15).
Also even before that ide-pci-generic.c driver has been available which is still more suitable than ide-generic.c one for CS5536 (it needs to be forced with "ide_pci_generic.all_generic_ide" kernel command line parameter).
Quoted 25 lines
Quoted 7 lines
Quoted 3 lines
Of course there are many reasons to use the correct PCI IDE host driver, but at the end, it's up to the user to choose which one he/she will use, and ideally any of them should work.
It happens to "work" on your system because CS5536 is relatively simple and non-buggy PCI IDE chipset.
Quoted 7 lines
In case of other PCI IDE chipsets you may not only be missing performance or some functionality - on some more quirky chipsets you may be also affecting data integrity (!).
Anyway you still can use ide-generic by using kernel parameters (just add "ide_generic.probe_mask=0x03" to your kernel command line).
As said before, this is a contract break between versions from, let's say, 2006 and today's.
Anyway, there's no way to avoid it, it we take the assumption above (designated drivers prior to ide-generic) as an axiom.
Quoted 3 lines
I agree that the ide-generic could be more verbose and print more information in case of skipping the probe (patches are welcomed).

Quoted 7 lines

Quoted 4 lines
I think this is a good idea, but more important IMHO is to document properly in the code all this discussion. If I have some spare time (not sure about this), I'll try to submit patches (better to have two) to address both "issues".
Thank you, it would be good to have this covered.

Two patches were prepared. The first one only documents (or tries to) this discussion, focusing the clear understanding of the issue, for developers. The second introduces new messages for the case where a PCI IDE device is detected and purposedly not grabbed by ide-device. It should help system admins to figure out more quickly what is going on and what should be done next.

I'll send them next, in two additional emails.

Please advise me if I'm proceeding in the proper way, as I'm quite new regarding LKMLs protocols.

Quoted 83 lines

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Moreover this patch introduces a regression. In the situation when there are no PCI IDE devices and the probing should be done automatically (for the first two legacy IDE ports) it will be no longer done.
Again, please elaborate more on this.
If there are no PCI IDE devices, well, there are no PCI IDE devices. Nothing is supposed to be found. Anyway, it seems they will be searched for, in ide_generic_check_pci_legacy_iobases(). Or I missed something important.
Quoted 8 lines
Quoted 4 lines
In case there are no PCI IDE devices we most likely are running on pre-PCI system and should be probing for legacy ISA IDE ports (please note they are are using the same I/O resources),
Ok, that is, ide-generic is tailored to pure ISA devices, and _can_ work in some PCI devices, although not guaranteed, on user request (probe_mask!=0).
Yes.
Quoted 43 lines
Quoted 17 lines
Quoted 17 lines
Quoted 16 lines
Quoted 4 lines
Quoted 3 lines
Now back to the using correct PCI IDE host drivers - Luiz what are the systems that you need this patch on? Could you please get 'lspci -nn' command output from them?
Quoted 8 lines
The original code before the patch in question probed the interfaces unconditionally, probe_mask was a static int set to "0x03".
Commit 20df429dd6671804999493baf2952f82582869fa changed the default behavior, as well as adding a new module parameter whose behavior makes no sense at all. Inverted bit logic? Give me a break.
Sorry, no, the fix is correct and I'm pushing it to Linus.
The "fix" is not correct and is not needed. 99% of users of ide-generic used it by mistake and should have used the designated host drivers for their hardware or PCI IDE generic host driver (not ide-generic one).
Quoted 8 lines
Quoted 3 lines
My system didn't work without this patch. I agree I could use the designated host driver, but it seems a little strong to say one _should_ use it.
Sorry but one should really use the designated drivers and we don't want to see any bugreports from using ide-generic on hardware that have designated drivers.
As said above, this is a kind of axiom, and I understand this is a conscious decision made somewhere in the past.
Avoiding extra bug reports is a smart decision, anyway.
Quoted 12 lines
Quoted 11 lines
Quoted 7 lines
Alan Cox did the work on fixing this for his pata_legacy libata host driver and later Borislav back-ported needed changes to ide-generic host driver in commit 20df429dd6671804999493baf2952f82582869fa (in *2008*).
Also the "fix" is not a revert but a new patch which introduces a real regression described by me in the previous mail.
As said above, it is a regression considering the versions from 2006. I agree that for someone who used the drivers from 2009 and after, it _may_ be a regression if she uses to bring all up with probe_mask=0.
Quoted 7 lines
Quoted 5 lines
The regression I was talking about is in the proposed patch.
By simply reversing the logic ISA IDE ports will no longer be probed automatically on non-PCI systems.
Understood. It makes sense.
Thank you for understanding.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Quoted 4 lines

--
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

Best regards,

Luiz