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.
On Tue, Dec 27, 2016 at 06:25:18PM +0100, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Tuesday, December 27, 2016 11:41:12 AM David Miller wrote:
> > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> > Date: Tue, 27 Dec 2016 17:06:19 +0100
> >
> > > On Tuesday, December 27, 2016 11:08:24 AM Bartlomiej Zolnierkiewicz wrote:
> > >>
> > >> Hi,
> > >>
> > >> On Monday, December 26, 2016 11:47:24 AM David Miller wrote:
> > >> > From: Luiz Carlos Ramos <lramos.prof@xxxxxxxxxxxx>
> > >> > Date: Tue, 11 Oct 2016 22:12:45 -0300
> > >> >
> > >> > > 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 <lramos.prof@xxxxxxxxxxxx>
> > >> >
> > >> > 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.
> > >>
> > >> > 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.
> > >> 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.
> > > 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.
> > > 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.
>
> 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.
>
> 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.
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
Best regards,
Luiz Carlos Ramos
São Paulo - Brazil
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
