Patchwork [05/20] pata_efar: always program master_data before slave_data

login
register
mail settings
Submitter Bartlomiej Zolnierkiewicz
Date Feb. 8, 2011, 12:24 p.m.
Message ID <20110208122409.19110.4233.sendpatchset@linux-mhg7.site>
Download mbox | patch
Permalink /patch/82320/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Bartlomiej Zolnierkiewicz - Feb. 8, 2011, 12:24 p.m.
From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 8 Feb 2011 12:39:25 +0100
Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data

We may need to set SITRE before programming slave_data.

This makes pata_efar match the behavior of IDE's slc90e66 host driver
and also of libata's ata_piix one.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/pata_efar.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
Alan Cox - Feb. 8, 2011, 1:07 p.m.
On Tue, 08 Feb 2011 13:24:09 +0100
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> >From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Tue, 8 Feb 2011 12:39:25 +0100
> Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data
> 
> We may need to set SITRE before programming slave_data.

Do you have a documentation cite for this or is it random fiddling with a
driver you can't test ?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 8, 2011, 1:16 p.m.
On Tue, Feb 8, 2011 at 2:07 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Tue, 08 Feb 2011 13:24:09 +0100
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
>
>> >From 8acc9371550ce5f98da56f6888ae4b898096ed2c Mon Sep 17 00:00:00 2001
>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> Date: Tue, 8 Feb 2011 12:39:25 +0100
>> Subject: [PATCH 05/20] pata_efar: always program master_data before slave_data
>>
>> We may need to set SITRE before programming slave_data.
>
> Do you have a documentation cite for this or is it random fiddling with a
> driver you can't test ?

It is good to enable SITRE register before programming it and all
Intel controllers
(from which EFAR chipset is derived) are programmed this way (per ata_piix.c).
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 8, 2011, 1:25 p.m.
> >> We may need to set SITRE before programming slave_data.
> >
> > Do you have a documentation cite for this or is it random fiddling with a
> > driver you can't test ?
> 
> It is good to enable SITRE register before programming it and all
> Intel controllers

That sounds like someone quoting religion. Documentation cite please.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 8, 2011, 1:28 p.m.
On Tue, Feb 8, 2011 at 2:25 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> >> We may need to set SITRE before programming slave_data.
>> >
>> > Do you have a documentation cite for this or is it random fiddling with a
>> > driver you can't test ?
>>
>> It is good to enable SITRE register before programming it and all
>> Intel controllers
>
> That sounds like someone quoting religion. Documentation cite please.

s/religion/common sense/
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Feb. 8, 2011, 1:38 p.m.
Hello.

On 08-02-2011 16:25, Alan Cox wrote:

>>>> We may need to set SITRE before programming slave_data.

>>> Do you have a documentation cite for this or is it random fiddling with a
>>> driver you can't test ?

>> It is good to enable SITRE register before programming it and all
>> Intel controllers

> That sounds like someone quoting religion. Documentation cite please.

    SLC90E66 datasheet only says that SIDETIM register has no effect without 
SITRE bit set.

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 8, 2011, 1:39 p.m.
> > That sounds like someone quoting religion. Documentation cite please.
> 
> s/religion/common sense/

Anyone who programs ATA controllers on the basis of common sense rather
than documentation, errata sheets and actually testing rather than
speculating is naïve. In the case of ATA more so than most hardware.

Stil waiting a documentation cite for your otherwise gratuitious and
untested change
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 8, 2011, 1:57 p.m.
On Tue, Feb 8, 2011 at 2:39 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> > That sounds like someone quoting religion. Documentation cite please.
>>
>> s/religion/common sense/
>
> Anyone who programs ATA controllers on the basis of common sense rather
> than documentation, errata sheets and actually testing rather than
> speculating is naïve. In the case of ATA more so than most hardware.

Fully agreed.

> Stil waiting a documentation cite for your otherwise gratuitious and
> untested change

Alan, I'm not doing real-time consulting with you.  The code is
provided in the best effort manner and it most likely will be
underrated (like all ATA/IDE work is) anyway so I do not really care
that much if it will be merged ever.  Once it will be merged it may
only mean more unpaid support time for me since some maintainers are
not doing any real maintenance work these days..   Please at least
respect some of my time spent on all this burdensome silly little
driver differences comparisons and read the whole patch set before
making comments on individual changes (which you certainly haven't
done given timing of your review mails and complexity of changes)..
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 8, 2011, 2:12 p.m.
> respect some of my time spent on all this burdensome silly little
> driver differences comparisons and read the whole patch set before
> making comments on individual changes (which you certainly haven't
> done given timing of your review mails and complexity of changes)..

I was hoping you'd improved but apparently not.

Any untested change is dangerous. An untested change that merges drivers
together simply means you can break lots of stuff for no gain at all.

If these were all PCI card devices it might make some sense but given
they are all motherboard chipsets putting them into one driver merely
increases memory use as well.

As far as stuff like 

   unsigned int has_sitre  = (dev->vendor != 0x8086 ||
                                  dev->device != 0x1230);

and the even worse mess you generate with the added patch all the other
PIIX code does this by flags, and if you had a HAS_SITRE (or NO_SITRE)
flag in the device data it would be obvious to anyone reading stuff how
it all fitted together.

Alan


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 8, 2011, 2:32 p.m.
On Tue, Feb 8, 2011 at 3:12 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> respect some of my time spent on all this burdensome silly little
>> driver differences comparisons and read the whole patch set before
>> making comments on individual changes (which you certainly haven't
>> done given timing of your review mails and complexity of changes)..
>
> I was hoping you'd improved but apparently not.

I'm not the person who doesn't change..

> Any untested change is dangerous. An untested change that merges drivers
> together simply means you can break lots of stuff for no gain at all.

This is why patches were posted to mailing list with a request for a
real hardware testing:

"All testing was done using QEMU's PIIX3 controller emulation so any testing
with real EFAR, IT8213, old PIIX, RDC and Radisys R82600 PATA controllers
would be really appreciated.."

instead of request for a merge.  It was all there in initial mail.

Besides decreased complexity,1400 LOC gone and 5 drivers removed
cannot be really called "no gain at all".

> If these were all PCI card devices it might make some sense but given
> they are all motherboard chipsets putting them into one driver merely
> increases memory use as well.

We can improve situation here by doing generic ATA or SCSI changes
instead, not worth keeping separate drivers when you make similar
effect but more maintainable and generic changes.

> As far as stuff like
>
>    unsigned int has_sitre  = (dev->vendor != 0x8086 ||
>                                   dev->device != 0x1230);
>
> and the even worse mess you generate with the added patch all the other
> PIIX code does this by flags, and if you had a HAS_SITRE (or NO_SITRE)
> flag in the device data it would be obvious to anyone reading stuff how
> it all fitted together.

Now that's what is a real review work.  It will be changed in the next
revision, thanks!

-- Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Feb. 10, 2011, 2:23 p.m.
Hello.

On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote:
[...]

> We may need to set SITRE before programming slave_data.

> This makes pata_efar match the behavior of IDE's slc90e66 host driver
> and also of libata's ata_piix one.

> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
[...]

> diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c
> index 1e2ff7d..7f564d7 100644
> --- a/drivers/ata/pata_efar.c
> +++ b/drivers/ata/pata_efar.c
> @@ -74,10 +74,12 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
>   			     u8 pio, bool use_mwdma)
>   {
>   	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
> +	unsigned int is_slave	= (adev->devno != 0);

    What's the point of this variable? To save one pointer dereference? :-)

>   	unsigned long flags;
>   	u8 master_port		= ap->port_no ? 0x42 : 0x40;
>   	u16 master_data;
>   	u8 udma_enable;
> +	u8 slave_data;
>   	int control = 0;
>
>   	/*
> @@ -110,14 +112,13 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
>   	pci_read_config_word(dev, master_port,&master_data);
>
>   	/* Set PPE, IE, and TIME as appropriate */
> -	if (adev->devno == 0) {
> +	if (is_slave == 0) {

    !is_slave appeals more to me. :-)

> @@ -126,12 +127,14 @@ static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
>   		pci_read_config_byte(dev, 0x44,&slave_data);
>   		slave_data&= ap->port_no ? 0x0F : 0xF0;
>   		slave_data |= ((timings[pio][0]<<  2) | timings[pio][1])<<  shift;
> -		pci_write_config_byte(dev, 0x44, slave_data);
>   	}
>
>   	master_data |= 0x4000;	/* Ensure SITRE is set */
>   	pci_write_config_word(dev, master_port, master_data);
>
> +	if (is_slave)
> +		pci_write_config_byte(dev, 0x44, slave_data);
> +
>   	pci_read_config_byte(dev, 0x48,&udma_enable);
>   	udma_enable&= ~(1<<  (2 * ap->port_no + adev->devno));
>   	pci_write_config_byte(dev, 0x48, udma_enable);

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 10, 2011, 5:14 p.m.
Hi,

On Thu, Feb 10, 2011 at 3:23 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote:
> [...]
>
>> We may need to set SITRE before programming slave_data.
>
>> This makes pata_efar match the behavior of IDE's slc90e66 host driver
>> and also of libata's ata_piix one.
>
>> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
>
> [...]
>
>> diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c
>> index 1e2ff7d..7f564d7 100644
>> --- a/drivers/ata/pata_efar.c
>> +++ b/drivers/ata/pata_efar.c
>> @@ -74,10 +74,12 @@ static void efar_set_timings(struct ata_port *ap,
>> struct ata_device *adev,
>>                             u8 pio, bool use_mwdma)
>>  {
>>        struct pci_dev *dev     = to_pci_dev(ap->host->dev);
>> +       unsigned int is_slave   = (adev->devno != 0);
>
>   What's the point of this variable? To save one pointer dereference? :-)

Make code more similar to ata_piix.c and thus easier for comparisons
through 'diff -ub'.

In reality it doesn't matter now that much as pata_efar (same for
pata_oldpiix) vanishes completely at the end of the patch series.. :-)

Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Feb. 10, 2011, 5:55 p.m.
Hello.

Bartlomiej Zolnierkiewicz wrote:

>> On 08-02-2011 15:24, Bartlomiej Zolnierkiewicz wrote:
>> [...]

>>> We may need to set SITRE before programming slave_data.
>>> This makes pata_efar match the behavior of IDE's slc90e66 host driver
>>> and also of libata's ata_piix one.

>>> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
>> [...]

>>> diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c
>>> index 1e2ff7d..7f564d7 100644
>>> --- a/drivers/ata/pata_efar.c
>>> +++ b/drivers/ata/pata_efar.c
>>> @@ -74,10 +74,12 @@ static void efar_set_timings(struct ata_port *ap,
>>> struct ata_device *adev,
>>>                             u8 pio, bool use_mwdma)
>>>  {
>>>        struct pci_dev *dev     = to_pci_dev(ap->host->dev);
>>> +       unsigned int is_slave   = (adev->devno != 0);

>>   What's the point of this variable? To save one pointer dereference? :-)

> Make code more similar to ata_piix.c and thus easier for comparisons
> through 'diff -ub'.

> In reality it doesn't matter now that much as pata_efar (same for
> pata_oldpiix) vanishes completely at the end of the patch series.. :-)

    Yeah, I understood that. Just don't have time to review the full series yet.

> Thanks,
> Bartlomiej

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 19, 2011, 9:25 a.m.
Hi,

On Tue, Feb 8, 2011 at 2:38 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 08-02-2011 16:25, Alan Cox wrote:
>
>>>>> We may need to set SITRE before programming slave_data.
>
>>>> Do you have a documentation cite for this or is it random fiddling with
>>>> a
>>>> driver you can't test ?
>
>>> It is good to enable SITRE register before programming it and all
>>> Intel controllers
>
>> That sounds like someone quoting religion. Documentation cite please.
>
>   SLC90E66 datasheet only says that SIDETIM register has no effect without
> SITRE bit set.

Alan, is this explanation sufficient to ACK this patch?  (Thanks
Sergei for digging it out.)

Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there
are no further concerns with them (thus leaving the merging of
PIIX-like drivers for later)?  They got additional testing on ICH4 and
they look mostly safe & straight-forward compared to #16-21.

Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 19, 2011, 4:48 p.m.
> >   SLC90E66 datasheet only says that SIDETIM register has no effect without
> > SITRE bit set.

Which means the current setup is fine yes ?

> Alan, is this explanation sufficient to ACK this patch?  (Thanks
> Sergei for digging it out.)
> 
> Jeff, would it be possible to queue patches #01-15 for 2.6.39 

Have they been tested on the relevant hardware yet - I don't believe the
changes should be made for the untested stuff and you indicated you
agreed with that.

The tidyups for the easily tested cases (PIIX4 etc) look sensible
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 20, 2011, 10:38 a.m.
On Sat, Feb 19, 2011 at 5:48 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> >   SLC90E66 datasheet only says that SIDETIM register has no effect without
>> > SITRE bit set.
>
> Which means the current setup is fine yes ?

That's the problem, the current setup is buggy in slave-only setups:

                slave_data |= ((timings[pio][0] << 2) |
timings[pio][1]) << shift;
                pci_write_config_byte(dev, 0x44, slave_data);
        }

        idetm_data |= 0x4000;   /* Ensure SITRE is set */
        pci_write_config_word(dev, idetm_port, idetm_data);

idetm_port needs to be programmed _before_ slave_data to "ensure SITRE is set".

IOW We currently depend on the BIOS here to set SITRE bit for us..

>> Alan, is this explanation sufficient to ACK this patch?  (Thanks
>> Sergei for digging it out.)
>>
>> Jeff, would it be possible to queue patches #01-15 for 2.6.39
>
> Have they been tested on the relevant hardware yet - I don't believe the
> changes should be made for the untested stuff and you indicated you
> agreed with that.

Unless they are obvious or risk is higher than benefit (bugfixes based
on code review).  I don't think that ATA code has became recently
special in this regard compared to the rest of the kernel.

> The tidyups for the easily tested cases (PIIX4 etc) look sensible

Please take look at the changes #01-15 then:

     ata_piix: SITRE handling fix
     ata_piix: unify code for programming PIO and MWDMA timings
     pata_efar: fix register naming used in efar_set_piomode()
     pata_efar: unify code for programming PIO and MWDMA timings
     pata_efar: always program master_data before slave_data
     pata_it8213: fix register naming used in it8213_set_piomode()
     pata_it8213: unify code for programming PIO and MWDMA timings
     pata_it8213: add UDMA100 and UDMA133 support
     pata_oldpiix: unify code for programming PIO and MWDMA timings
     pata_radisys: unify code for programming PIO and MWDMA timings
     pata_rdc: unify code for programming PIO and MWDMA timings
     pata_rdc: parallel scanning needs an extra locking
     pata_rdc: add Power Management support
     pata_oldpiix: add locking for parallel scanning
     pata_oldpiix: enable parallel scan

Most patches has been posted months ago in the past as the part of
atang tree.  They don't contain risky stuff (it was intentionally
pushed at the end of the patch queue).  We have here obvious register
naming unification, PIO/DMA folding (it was tested with ata_piix and
only later ported to other PIIX-like drivers) and:

* para_rdc: locking bug-fix and Power Management support

* pata_it8213: IDE -> libata regression fix (UDMA100/133 support based
on vendor / old IDE driver)

* pata_oldpiix: locking bug-fix (ACK-ed by you in the past) and
parallel scanning support

I see a lot of magnitude more risky stuff going elsewhere in the
kernel, including ATA itself and if there are any concerns left those
patches (#01-15) I'll be happy to address them.

Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 20, 2011, 11:36 a.m.
>         idetm_data |= 0x4000;   /* Ensure SITRE is set */
>         pci_write_config_word(dev, idetm_port, idetm_data);
> 
> idetm_port needs to be programmed _before_ slave_data to "ensure SITRE is set".

That isn't what the documentation seems to say. It says it has no effect
unless SITRE is set not that you can't program the registers.

> Unless they are obvious or risk is higher than benefit (bugfixes based
> on code review).  I don't think that ATA code has became recently
> special in this regard compared to the rest of the kernel.

They aren't special - random unneeded code changes that haven't been
tested shouldn't be going into the code unless there is a big gain. For
obscure ancient devices there isn't a gain.

>      ata_piix: unify code for programming PIO and MWDMA timings

Which as I said makes sense

>      pata_efar: fix register naming used in efar_set_piomode()
>      pata_efar: unify code for programming PIO and MWDMA timings
>      pata_efar: always program master_data before slave_data

All untested

>      pata_it8213: fix register naming used in it8213_set_piomode()
>      pata_it8213: unify code for programming PIO and MWDMA timings
>      pata_it8213: add UDMA100 and UDMA133 support

All untested

>      pata_oldpiix: unify code for programming PIO and MWDMA timings

Untested

>      pata_radisys: unify code for programming PIO and MWDMA timings

Untested

>      pata_rdc: unify code for programming PIO and MWDMA timings
>      pata_rdc: parallel scanning needs an extra locking
>      pata_rdc: add Power Management support

All untested but the locking is a clear bug fix and I think should go in

>      pata_oldpiix: add locking for parallel scanning
>      pata_oldpiix: enable parallel scan

This is an ancient device on some old pentium class boxes, it's not
worth adding this stuff really is it ? Well maybe putting the locking in
or at least comments that it will be needed ?

> Most patches has been posted months ago in the past as the part of
> atang tree.

So - where are the test reports

> * pata_it8213: IDE -> libata regression fix (UDMA100/133 support based
> on vendor / old IDE driver)

I didn't see it tested in the old IDE driver either.

> * pata_oldpiix: locking bug-fix (ACK-ed by you in the past) and
> parallel scanning support

I'm happy with the locking fix, I don't see the point in the parallel
scan - and that wants testing because I don't know how the hardware will
react. Most controllers were not designed for parallel scan and its a
path windows will never have exercised therefore may well never have been
tested out.

In the PIIX4+ cases Jeff insisted we chased this down with the hardware
folks in Intel to be sure it was ok. I'm not sure there is anyone who
remembers original PIIX however.

> I see a lot of magnitude more risky stuff going elsewhere in the
> kernel, including ATA itself

And being tested.

efar/it8213/radisys are going to be tricky to find testers for as they
are rare chips but the RDC is found in some of the embedded
CPU/ATA/Net/etc in a device embedded x86 devices.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Feb. 21, 2011, 8:06 p.m.
On 02/19/2011 04:25 AM, Bartlomiej Zolnierkiewicz wrote:
> Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there
> are no further concerns with them (thus leaving the merging of
> PIIX-like drivers for later)?  They got additional testing on ICH4 and
> they look mostly safe&  straight-forward compared to #16-21.

This seems to directly contradict what you wrote earlier in the thread,

	This is why patches were posted to mailing list with a request
	for a real hardware testing:

	"All testing was done using QEMU's PIIX3 controller emulation
	so any testing with real EFAR, IT8213, old PIIX, RDC and
	Radisys R82600 PATA controllers would be really appreciated.."

	instead of request for a merge.  It was all there in initial
	mail.

and

	I do not really care that much if it will be merged ever

Regardless of this self-contradictory attitude, I do want useful patches 
and many of these patches seem useful.

So I will continue watching the Bart/Alan/Sergei threads play out, and 
then look at merging the result.  In the midst of all the arguing, 
productive work / forward progress is occurring, so the end result 
should be positive.

It would be nice if we could get at least an "it works" test for the 
older hardware, since those are the changes /least/ likely to be tested 
by queueing to linux-next.

	Jeff


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 22, 2011, 9:19 a.m.
Hi,

On Mon, Feb 21, 2011 at 9:06 PM, Jeff Garzik <jgarzik@pobox.com> wrote:
> On 02/19/2011 04:25 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Jeff, would it be possible to queue patches #01-15 for 2.6.39 if there
>> are no further concerns with them (thus leaving the merging of
>> PIIX-like drivers for later)?  They got additional testing on ICH4 and
>> they look mostly safe&  straight-forward compared to #16-21.
>
> This seems to directly contradict what you wrote earlier in the thread,
>
>        This is why patches were posted to mailing list with a request
>        for a real hardware testing:
>
>        "All testing was done using QEMU's PIIX3 controller emulation
>        so any testing with real EFAR, IT8213, old PIIX, RDC and
>        Radisys R82600 PATA controllers would be really appreciated.."
>
>        instead of request for a merge.  It was all there in initial
>        mail.
>
> and
>
>        I do not really care that much if it will be merged ever
>
> Regardless of this self-contradictory attitude, I do want useful patches and
> many of these patches seem useful.

Nothing self-contradictory there. :)

First quote is about patches #01-15 only, not whole patchset (#01-20)
like the second one, and I still don't care _that_ much personally if
it gets merged since it is all unpaid & voluntary work.

> So I will continue watching the Bart/Alan/Sergei threads play out, and then
> look at merging the result.  In the midst of all the arguing, productive
> work / forward progress is occurring, so the end result should be positive.
>
> It would be nice if we could get at least an "it works" test for the older
> hardware, since those are the changes /least/ likely to be tested by
> queueing to linux-next.

I was thinking about re-doing ata_piix part in a way that we could
merge it now by adding support for older PIIX-alikes to ata_piix and
making it enabled only if "all_piixalikes" module parameter is
specified.   This way older drivers would be left untouched for now
and we can easily get in-tree testing for a new code.  Does it sound
as a viable alternative?

Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 22, 2011, 11:14 a.m.
> I was thinking about re-doing ata_piix part in a way that we could
> merge it now by adding support for older PIIX-alikes to ata_piix and
> making it enabled only if "all_piixalikes" module parameter is
> specified.   This way older drivers would be left untouched for now
> and we can easily get in-tree testing for a new code.  Does it sound
> as a viable alternative?

That is the bit to me that makes the least sense. Each of the devices in
question is found only in the chipset so they can never be in combination
on a board (except multiple PIIX4 which is already covered)

Merging them makes it more likely stuff breaks and increases memory usage
- its a lose/lose situation.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 23, 2011, 8:45 a.m.
On Tue, Feb 22, 2011 at 12:14 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> I was thinking about re-doing ata_piix part in a way that we could
>> merge it now by adding support for older PIIX-alikes to ata_piix and
>> making it enabled only if "all_piixalikes" module parameter is
>> specified.   This way older drivers would be left untouched for now
>> and we can easily get in-tree testing for a new code.  Does it sound
>> as a viable alternative?
>
> That is the bit to me that makes the least sense. Each of the devices in
> question is found only in the chipset so they can never be in combination
> on a board (except multiple PIIX4 which is already covered)
>
> Merging them makes it more likely stuff breaks and increases memory usage
> - its a lose/lose situation.

Each device is very similar to other one, so keeping 6 drivers for the
(almost) same stuff makes a little sense from the long-term
maintainability POV,  It is a question of different trade-offs since
on the other side of equation we have 5 drivers less, 1400 LOCs less,
no risk of losing bugfixes and features (which is true already w/
pata_rdc lacking locking fixes + Power Management and pata_oldpiix
lacking parallel scanning feature).

As for increased memory usage -- we are talking here only about 10-20k
more.  If it really is a problem maybe ata_piix can be redesigned into
ata_generic-style manner so with the help of existing config options
we can keep code size / memory usage on a existing level.

Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Feb. 23, 2011, 8:53 a.m.
On Wed, Feb 23, 2011 at 9:45 AM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:

> As for increased memory usage -- we are talking here only about 10-20k
> more.  If it really is a problem maybe ata_piix can be redesigned into
> ata_generic-style manner so with the help of existing config options
> we can keep code size / memory usage on a existing level.

s/ata_generic/pata_legacy/ of course -- what I have in mind is making
Intel specific code depended on ATA_PIIX and adding new config option
ATA_PIIXALIKE for generic code which would be selected by existing
PATA_[EFAR,IT8213, OLDPIIX,RADISYS,RDC] options.

Thanks,
Bartlomiej
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Feb. 23, 2011, 9:14 a.m.
> on the other side of equation we have 5 drivers less, 1400 LOCs less,
> no risk of losing bugfixes and features (which is true already w/
> pata_rdc lacking locking fixes + Power Management and pata_oldpiix
> lacking parallel scanning feature).

The parallel scanning feature is somewhat irrelevant on an original PIIX,
if the hardware even supports it which is somewhat of an unknown. If you
have a minor boot time performance concern you'd buy a real computer
instead.

And the other stuff you actually create a problem rather than solving one
- the difficulting testing the rare chips makes it harder to fix ata_piix
bugs and do full testing.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/pata_efar.c b/drivers/ata/pata_efar.c
index 1e2ff7d..7f564d7 100644
--- a/drivers/ata/pata_efar.c
+++ b/drivers/ata/pata_efar.c
@@ -74,10 +74,12 @@  static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
 			     u8 pio, bool use_mwdma)
 {
 	struct pci_dev *dev	= to_pci_dev(ap->host->dev);
+	unsigned int is_slave	= (adev->devno != 0);
 	unsigned long flags;
 	u8 master_port		= ap->port_no ? 0x42 : 0x40;
 	u16 master_data;
 	u8 udma_enable;
+	u8 slave_data;
 	int control = 0;
 
 	/*
@@ -110,14 +112,13 @@  static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
 	pci_read_config_word(dev, master_port, &master_data);
 
 	/* Set PPE, IE, and TIME as appropriate */
-	if (adev->devno == 0) {
+	if (is_slave == 0) {
 		master_data &= 0xCCF0;
 		master_data |= control;
 		master_data |= (timings[pio][0] << 12) |
 			(timings[pio][1] << 8);
 	} else {
 		int shift = 4 * ap->port_no;
-		u8 slave_data;
 
 		master_data &= 0xFF0F;
 		master_data |= (control << 4);
@@ -126,12 +127,14 @@  static void efar_set_timings(struct ata_port *ap, struct ata_device *adev,
 		pci_read_config_byte(dev, 0x44, &slave_data);
 		slave_data &= ap->port_no ? 0x0F : 0xF0;
 		slave_data |= ((timings[pio][0] << 2) | timings[pio][1]) << shift;
-		pci_write_config_byte(dev, 0x44, slave_data);
 	}
 
 	master_data |= 0x4000;	/* Ensure SITRE is set */
 	pci_write_config_word(dev, master_port, master_data);
 
+	if (is_slave)
+		pci_write_config_byte(dev, 0x44, slave_data);
+
 	pci_read_config_byte(dev, 0x48, &udma_enable);
 	udma_enable &= ~(1 << (2 * ap->port_no + adev->devno));
 	pci_write_config_byte(dev, 0x48, udma_enable);