[PATCHv3 2/2] Add force_epp module option for parport_pc.

[PATCHv3 2/2] Add force_epp module option for parport_pc.

[Matwey V. Kornilov]

>From cf37d0cc4d51da5c0b368e1f5ab05082c041d1e1 Mon Sep 17 00:00:00 2001
From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>
Date: Wed, 25 Jun 2014 01:08:45 +0400
Subject: [PATCHv3 2/2] Add force_epp module option for parport_pc.

The detection of Intel EPP bug is known to produce much false positives.
The new option is introduced to force enable EPP in spite of the test result.

Tested-by: Heiko Andreas Sommer <hsommer@eso.org>
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
  drivers/parport/parport_pc.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
index a6eaafb..fb7530d 100644
--- a/drivers/parport/parport_pc.c
+++ b/drivers/parport/parport_pc.c
@@ -105,6 +105,9 @@ static int user_specified;
         (defined(CONFIG_PARPORT_1284) && defined(CONFIG_PARPORT_PC_FIFO))
  static int verbose_probing;
  #endif
+#ifdef CONFIG_PARPORT_1284
+static int force_epp;
+#endif
  static int pci_registered_parport;
  static int pnp_registered_parport;

@@ -1764,7 +1767,7 @@ static int parport_EPP_supported(struct parport *pb)
  		return 0;  /* No way to clear timeout */

  	/* Check for Intel bug. */
-	if (intel_bug_present(pb))
+	if (!force_epp && intel_bug_present(pb))
  		return 0;

  	pb->modes |= PARPORT_MODE_EPP;
@@ -3147,6 +3150,10 @@ module_param_array(dma, charp, NULL, 0);
  MODULE_PARM_DESC(verbose_probing, "Log chit-chat during initialisation");
  module_param(verbose_probing, int, 0644);
  #endif
+#ifdef CONFIG_PARPORT_1284
+MODULE_PARM_DESC(force_epp, "Force EPP enabling when buggy hardware found by the module checks");
+module_param(force_epp, int, 0);
+#endif
  #ifdef CONFIG_PCI
  static char *init_mode;
  MODULE_PARM_DESC(init_mode,
-- 
1.8.1.4

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Greg KH]

On Mon, Jul 07, 2014 at 11:01:51AM +0400, Matwey V. Kornilov wrote:
> >From cf37d0cc4d51da5c0b368e1f5ab05082c041d1e1 Mon Sep 17 00:00:00 2001
> From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>
> Date: Wed, 25 Jun 2014 01:08:45 +0400
> Subject: [PATCHv3 2/2] Add force_epp module option for parport_pc.
> 
> The detection of Intel EPP bug is known to produce much false positives.
> The new option is introduced to force enable EPP in spite of the test result.

module parameters are horrid, how is someone supposed to know to use
this?  Why can't we "fix" the detection logic?  You just now broke
systems that were working by forcing them to now set a module option
where previously they didn't, so I can't take these patches as-is,
sorry.

greg k-h

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Matwey V. Kornilov]

On Wed, 9 Jul 2014, Greg KH wrote:

> On Mon, Jul 07, 2014 at 11:01:51AM +0400, Matwey V. Kornilov wrote:
>>> From cf37d0cc4d51da5c0b368e1f5ab05082c041d1e1 Mon Sep 17 00:00:00 2001
>> From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>
>> Date: Wed, 25 Jun 2014 01:08:45 +0400
>> Subject: [PATCHv3 2/2] Add force_epp module option for parport_pc.
>>
>> The detection of Intel EPP bug is known to produce much false positives.
>> The new option is introduced to force enable EPP in spite of the test result.

Hi,

First of all, maybe I missed something fundamental, or did something 
wrong, but I can't understand how is it going to break working systems? We 
have current behaviour B0, and introduce an option switches between B0 
(which is default value) and alternative behaviour B1. So, until you go to 
/etc/modprobe.d to override defaults or modprobe with specific option the 
current behaviour B0 is active. Am I not right here?

> module parameters are horrid, how is someone supposed to know to use
> this?

I usually do `modinfo' to list existing options. Do I understand right 
that description has to be added into Documentations also?

> Why can't we "fix" the detection logic?

Initial check had been introduced in 1999 [1], however I am failed to find 
any proves, because davej/history.git is missed. Initial committer is 
unknown. The check fixes issue with the affected hardware (A) and 
introduces new one with false-positive (F) hardware.

Now, there are no evidences of what hardware is affected (it seems to be 
quite aged), and no owners of this hardware are known. So, If I had fixed 
the detection logic (in one or another way), I would have not been able to 
check it.

What I am trying to do here (as it is described in 0/2) is to allow people 
with false-positive hardware somehow operate with it.

> You just now broke systems that were working by forcing them to now set a module option
> where previously they didn't

See above.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=630593#79

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Dave Jones]

On Thu, Jul 10, 2014 at 11:56:15AM +0400, Matwey V. Kornilov wrote:
 
 > Initial check had been introduced in 1999 [1], however I am failed to find 
 > any proves, because davej/history.git is missed.

https://archive.org/details/git-history-of-linux

	Dave

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Matwey V. Kornilov]

2014-07-10 18:11 GMT+04:00 Dave Jones <davej@redhat.com>:
> On Thu, Jul 10, 2014 at 11:56:15AM +0400, Matwey V. Kornilov wrote:
>
>  > Initial check had been introduced in 1999 [1], however I am failed to find
>  > any proves, because davej/history.git is missed.
>
> https://archive.org/details/git-history-of-linux

Thank you, Dave.

This was introduced by cecd99566ac6aba844ff296e461f88131013da94 (2.3.10).
In lkml archives I've found that this code is probably belongs to
David Campbell, whose email has been dropped as bounced. So, I don't
have any idea whether we can contact him.

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Greg KH]

On Thu, Jul 10, 2014 at 11:56:15AM +0400, Matwey V. Kornilov wrote:
> 
> 
> On Wed, 9 Jul 2014, Greg KH wrote:
> 
> >On Mon, Jul 07, 2014 at 11:01:51AM +0400, Matwey V. Kornilov wrote:
> >>>From cf37d0cc4d51da5c0b368e1f5ab05082c041d1e1 Mon Sep 17 00:00:00 2001
> >>From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>
> >>Date: Wed, 25 Jun 2014 01:08:45 +0400
> >>Subject: [PATCHv3 2/2] Add force_epp module option for parport_pc.
> >>
> >>The detection of Intel EPP bug is known to produce much false positives.
> >>The new option is introduced to force enable EPP in spite of the test result.
> 
> Hi,
> 
> First of all, maybe I missed something fundamental, or did something wrong,
> but I can't understand how is it going to break working systems?

I thought you disabled the quirk test and now rely on the module option
instead.  That would require a machine that was happily relying on the
quirk test to now be forced to add a module option, right?

Or did I read the patch incorrectly?

Why not implement Alan's suggestion?

thanks,

greg k-h

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Matwey V. Kornilov]

2014-07-10 21:09 GMT+04:00 Greg KH <gregkh@linuxfoundation.org>:
> On Thu, Jul 10, 2014 at 11:56:15AM +0400, Matwey V. Kornilov wrote:
>>
>>
>> On Wed, 9 Jul 2014, Greg KH wrote:
>>
>> >On Mon, Jul 07, 2014 at 11:01:51AM +0400, Matwey V. Kornilov wrote:
>> >>>From cf37d0cc4d51da5c0b368e1f5ab05082c041d1e1 Mon Sep 17 00:00:00 2001
>> >>From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>
>> >>Date: Wed, 25 Jun 2014 01:08:45 +0400
>> >>Subject: [PATCHv3 2/2] Add force_epp module option for parport_pc.
>> >>
>> >>The detection of Intel EPP bug is known to produce much false positives.
>> >>The new option is introduced to force enable EPP in spite of the test result.
>>
>> Hi,
>>
>> First of all, maybe I missed something fundamental, or did something wrong,
>> but I can't understand how is it going to break working systems?
>
> I thought you disabled the quirk test and now rely on the module option
> instead.  That would require a machine that was happily relying on the
> quirk test to now be forced to add a module option, right?

No, this would not...

> Or did I read the patch incorrectly?

Maybe I've implemented something incorrectly? I think I suggested
exactly inverse thing: the check is disabled only when the option is
touched by user:

!force_epp && intel_bug_present(pb) <=> intel_bug_present(pb) (given
that force_epp is false)

> Why not implement Alan's suggestion?

Why not, if you are fine with it. Are you sure that PPro was turning point?

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Greg KH]

On Thu, Jul 10, 2014 at 09:32:59PM +0400, Matwey V. Kornilov wrote:
> 2014-07-10 21:09 GMT+04:00 Greg KH <gregkh@linuxfoundation.org>:
> > On Thu, Jul 10, 2014 at 11:56:15AM +0400, Matwey V. Kornilov wrote:
> >>
> >>
> >> On Wed, 9 Jul 2014, Greg KH wrote:
> >>
> >> >On Mon, Jul 07, 2014 at 11:01:51AM +0400, Matwey V. Kornilov wrote:
> >> >>>From cf37d0cc4d51da5c0b368e1f5ab05082c041d1e1 Mon Sep 17 00:00:00 2001
> >> >>From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>
> >> >>Date: Wed, 25 Jun 2014 01:08:45 +0400
> >> >>Subject: [PATCHv3 2/2] Add force_epp module option for parport_pc.
> >> >>
> >> >>The detection of Intel EPP bug is known to produce much false positives.
> >> >>The new option is introduced to force enable EPP in spite of the test result.
> >>
> >> Hi,
> >>
> >> First of all, maybe I missed something fundamental, or did something wrong,
> >> but I can't understand how is it going to break working systems?
> >
> > I thought you disabled the quirk test and now rely on the module option
> > instead.  That would require a machine that was happily relying on the
> > quirk test to now be forced to add a module option, right?
> 
> No, this would not...
> 
> > Or did I read the patch incorrectly?
> 
> Maybe I've implemented something incorrectly? I think I suggested
> exactly inverse thing: the check is disabled only when the option is
> touched by user:
> 
> !force_epp && intel_bug_present(pb) <=> intel_bug_present(pb) (given
> that force_epp is false)

I don't understand, care to just resend the patches? I really don't
remember what the patch said...

> > Why not implement Alan's suggestion?
> 
> Why not, if you are fine with it. Are you sure that PPro was turning point?

If Alan says so, I believe him :)

greg k-h

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Matwey V. Kornilov]

>>> Or did I read the patch incorrectly?
>>
>> Maybe I've implemented something incorrectly? I think I suggested
>> exactly inverse thing: the check is disabled only when the option is
>> touched by user:
>>
>> !force_epp && intel_bug_present(pb) <=> intel_bug_present(pb) (given
>> that force_epp is false)
>
> I don't understand, care to just resend the patches? I really don't
> remember what the patch said...

The patch is in bottom of this message. My suggestion was that people 
suffering from false positives will set the option to TRUE, and other 
won't touch it.

>>> Why not implement Alan's suggestion?
>>
>> Why not, if you are fine with it. Are you sure that PPro was turning point?
>
> If Alan says so, I believe him :)

Ok, then I believe you, I will go to reimplement it in Alan's way.


>From cf37d0cc4d51da5c0b368e1f5ab05082c041d1e1 Mon Sep 17 00:00:00 2001
From: "Matwey V. Kornilov" <matwey.kornilov@gmail.com>
Date: Wed, 25 Jun 2014 01:08:45 +0400
Subject: [PATCH 2/2] Add force_epp module option for parport_pc.

The detection of Intel EPP bug is known to produce much false positives.
The new option is introduced to force enable EPP in spite of the test result.

Tested-by: Heiko Andreas Sommer <hsommer@eso.org>
Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
---
  drivers/parport/parport_pc.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
index a6eaafb..fb7530d 100644
--- a/drivers/parport/parport_pc.c
+++ b/drivers/parport/parport_pc.c
@@ -105,6 +105,9 @@ static int user_specified;
         (defined(CONFIG_PARPORT_1284) && defined(CONFIG_PARPORT_PC_FIFO))
  static int verbose_probing;
  #endif
+#ifdef CONFIG_PARPORT_1284
+static int force_epp;
+#endif
  static int pci_registered_parport;
  static int pnp_registered_parport;

@@ -1764,7 +1767,7 @@ static int parport_EPP_supported(struct parport *pb)
  		return 0;  /* No way to clear timeout */

  	/* Check for Intel bug. */
-	if (intel_bug_present(pb))
+	if (!force_epp && intel_bug_present(pb))
  		return 0;

  	pb->modes |= PARPORT_MODE_EPP;
@@ -3147,6 +3150,10 @@ module_param_array(dma, charp, NULL, 0);
  MODULE_PARM_DESC(verbose_probing, "Log chit-chat during initialisation");
  module_param(verbose_probing, int, 0644);
  #endif
+#ifdef CONFIG_PARPORT_1284
+MODULE_PARM_DESC(force_epp, "Force EPP enabling when buggy hardware found by the module checks");
+module_param(force_epp, int, 0);
+#endif
  #ifdef CONFIG_PCI
  static char *init_mode;
  MODULE_PARM_DESC(init_mode,
-- 
1.8.1.4

Re: [Linux-parport] [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Leopold Palomo-Avellaneda]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

Hi,

this thread has been very interesting. I feel that I'm one of the 
"instigators" to solve this f... bug in the kernel. But, it seems that it's 
quiet difficult, more because the kernel developers than the bug itself. 

But well, to be honest, the kernel developers do a great and complex job, so 
probably they have good reasons to react in that way. Think that it's the 
second time that a patch to solve this bug is rejected.

Thanks Matwey for the patch and the afford to solve this bug. I have several 
boxes in the lab where I work (Dell 745) that suffer that false positives.

Also, there's some pci card that also suffer that. I think that we have one. 
But, I'm not sure.

So, I can check whatever could be necessary.

Best regards,

Leopold




-- 
--
Linux User 152692     PGP: 05F4A7A949A2D9AA
Catalonia
-------------------------------------
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Linux-parport] [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Matwey V. Kornilov]

Here usb-stick image with the new (Alan) version of the patches.

https://susestudio.com/download/1cbfcf5c4ca89206fc170f00b962be0a/openSUSE_13.1_parport_pc.i686-0.1.0.oem.tar.gz

login root:linux or tux:linux

Heiko, could you please also check that you see EPP?

2014-07-11 1:48 GMT+04:00 Leopold Palomo-Avellaneda <leo@alaxarxa.net>:
> Hi,
>
> this thread has been very interesting. I feel that I'm one of the
> "instigators" to solve this f... bug in the kernel. But, it seems that it's
> quiet difficult, more because the kernel developers than the bug itself.
>
> But well, to be honest, the kernel developers do a great and complex job, so
> probably they have good reasons to react in that way. Think that it's the
> second time that a patch to solve this bug is rejected.
>
> Thanks Matwey for the patch and the afford to solve this bug. I have several
> boxes in the lab where I work (Dell 745) that suffer that false positives.
>
> Also, there's some pci card that also suffer that. I think that we have one.
> But, I'm not sure.
>
> So, I can check whatever could be necessary.
>
> Best regards,
>
> Leopold
>
>
>
>
> --
> --
> Linux User 152692     PGP: 05F4A7A949A2D9AA
> Catalonia
> -------------------------------------
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[One Thousand Gnomes]

> module parameters are horrid, how is someone supposed to know to use
> this?  Why can't we "fix" the detection logic?  You just now broke
> systems that were working by forcing them to now set a module option
> where previously they didn't, so I can't take these patches as-is,
> sorry.

Can we not just add "if the CPU is PPro or higher than all is fine" ? to
the test ?

Alan

Re: [PATCHv3 2/2] Add force_epp module option for parport_pc.

[Greg KH]

On Thu, Jul 10, 2014 at 04:52:25PM +0100, One Thousand Gnomes wrote:
> > module parameters are horrid, how is someone supposed to know to use
> > this?  Why can't we "fix" the detection logic?  You just now broke
> > systems that were working by forcing them to now set a module option
> > where previously they didn't, so I can't take these patches as-is,
> > sorry.
> 
> Can we not just add "if the CPU is PPro or higher than all is fine" ? to
> the test ?

That sounds reasonable to me.

greg k-h