Who's responsible for configuring CLS on a cardbus device?

Who's responsible for configuring CLS on a cardbus device?

[Tejun Heo]

Hello,

This is regarding bko#13257.

 http://bugzilla.kernel.org/show_bug.cgi?id=13257

towerlexa@gmx.de was experiencing very slow transfer rate when using a
cardbus sata_sil SATA controller which is known to be sensitive to
cache line size setting.  The reset default is zero and no one
configured it causing poor performance.

This is solvable by simply setting CLS to the correct value but who's
job is it?  For non-hotplug devices, this is configured by the BIOS
(at least on PC), so for hotplug devices I think falls on the lap of
the PCI code but I'm not sure.  If this is something which the
sata_sil driver should be responsible for, is there an established way
to determine the proper CLS value?

Thanks.

-- 
tejun

Re: Who's responsible for configuring CLS on a cardbus device?

[Alan Cox]

On Tue, 26 May 2009 22:05:08 +0900
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> This is regarding bko#13257.
> 
>  http://bugzilla.kernel.org/show_bug.cgi?id=13257
> 
> towerlexa@gmx.de was experiencing very slow transfer rate when using a
> cardbus sata_sil SATA controller which is known to be sensitive to
> cache line size setting.  The reset default is zero and no one
> configured it causing poor performance.
> 
> This is solvable by simply setting CLS to the correct value but who's
> job is it?  For non-hotplug devices, this is configured by the BIOS
> (at least on PC), so for hotplug devices I think falls on the lap of
> the PCI code but I'm not sure.  If this is something which the
> sata_sil driver should be responsible for, is there an established way
> to determine the proper CLS value?

Currently its handled by pci_set_mwi() but there isn't actually a more
direct way to do this.

Alan

Re: Who's responsible for configuring CLS on a cardbus device?

[Robert Hancock]

Alan Cox wrote:
> On Tue, 26 May 2009 22:05:08 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> Hello,
>>
>> This is regarding bko#13257.
>>
>>  http://bugzilla.kernel.org/show_bug.cgi?id=13257
>>
>> towerlexa@gmx.de was experiencing very slow transfer rate when using a
>> cardbus sata_sil SATA controller which is known to be sensitive to
>> cache line size setting.  The reset default is zero and no one
>> configured it causing poor performance.
>>
>> This is solvable by simply setting CLS to the correct value but who's
>> job is it?  For non-hotplug devices, this is configured by the BIOS
>> (at least on PC), so for hotplug devices I think falls on the lap of
>> the PCI code but I'm not sure.  If this is something which the
>> sata_sil driver should be responsible for, is there an established way
>> to determine the proper CLS value?
> 
> Currently its handled by pci_set_mwi() but there isn't actually a more
> direct way to do this.

Yeah, I guess the assumption is that unless the device is using MWI it 
doesn't care about cache line size. However, in the case of the sata_sil 
controllers (and possibly other devices), the device cares about it for 
other purposes (I think it's FIFO handling in this case).

Maybe we should just be setting the cache line size somewhere more 
basic, like pci_set_master or something?

Re: Who's responsible for configuring CLS on a cardbus device?

[Tejun Heo]

Hello,

Robert Hancock wrote:
> Alan Cox wrote:
>> Currently its handled by pci_set_mwi() but there isn't actually a more
>> direct way to do this.

Thanks Alan.

> Yeah, I guess the assumption is that unless the device is using MWI it
> doesn't care about cache line size. However, in the case of the sata_sil
> controllers (and possibly other devices), the device cares about it for
> other purposes (I think it's FIFO handling in this case).
> 
> Maybe we should just be setting the cache line size somewhere more
> basic, like pci_set_master or something?

Hmmm... given that it is something which is usually handled by the
system firmware, wouldn't it be more fitting to configure it from pci
hotplug code?

Thanks.

-- 
tejun

Re: Who's responsible for configuring CLS on a cardbus device?

[Kenji Kaneshige]

Tejun Heo wrote:
> Hello,
> 
> Robert Hancock wrote:
>> Alan Cox wrote:
>>> Currently its handled by pci_set_mwi() but there isn't actually a more
>>> direct way to do this.
> 
> Thanks Alan.
> 
>> Yeah, I guess the assumption is that unless the device is using MWI it
>> doesn't care about cache line size. However, in the case of the sata_sil
>> controllers (and possibly other devices), the device cares about it for
>> other purposes (I think it's FIFO handling in this case).
>>
>> Maybe we should just be setting the cache line size somewhere more
>> basic, like pci_set_master or something?
> 
> Hmmm... given that it is something which is usually handled by the
> system firmware, wouldn't it be more fitting to configure it from pci
> hotplug code?
> 

I don't know cardbus devices at all, but Standard Hot-Plug Controller
driver ('shpchp') and PCI Express Hot-Plug controller driver ('pciehp')
configures cache line size of hot-added device. The cache line size is
gotten from firmware through ACPI _HPP or _HPX method.

Thanks,
Kenji Kaneshige

[RFC PATCH] pccard: configure CLS on attach

[Tejun Heo]

For non hotplug PCI devices, the system firmware usually configures
CLS correctly.  For pccard devices system firmware can't do it and
linux PCI layer doesn't do it either leaving it unconfigured.
Unfortunately this leads to poor performanc for certain devices
(sata_sil).  Unless MWI, which requires separate configuration, is to
be used, CLS doesn't affect correctness, so the configuration should
be harmless.

Please note that some other PCI hotplug drivers (shpchp and pciehp)
also configure CLS on hotplug.

THIS IS A RFC PATCH, SO NO SOB.  PLEASE DON'T APPLY YET.

towerlexa, can you please test this patch?

Cc: Daniel Ritz <daniel.ritz@gmx.ch>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Greg KH <greg@kroah.com>
Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Cc: towerlexa@gmx.de
---
 drivers/pci/pci.c        |    3 +--
 drivers/pcmcia/cardbus.c |   23 +++++++++++++++--------
 include/linux/pci.h      |    1 +
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1a91bf9..eafbe01 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1860,8 +1860,7 @@ u8 pci_cache_line_size = PCI_CACHE_LINE_BYTES / 4;
  *
  * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
  */
-static int
-pci_set_cacheline_size(struct pci_dev *dev)
+int pci_set_cacheline_size(struct pci_dev *dev)
 {
 	u8 cacheline_size;
 
diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
index db77e1f..98789c0 100644
--- a/drivers/pcmcia/cardbus.c
+++ b/drivers/pcmcia/cardbus.c
@@ -184,26 +184,33 @@ fail:
     
 =====================================================================*/
 
-/*
- * Since there is only one interrupt available to CardBus
- * devices, all devices downstream of this device must
- * be using this IRQ.
- */
-static void cardbus_assign_irqs(struct pci_bus *bus, int irq)
+static void cardbus_config_irq_and_cls(struct pci_bus *bus, int irq)
 {
 	struct pci_dev *dev;
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		u8 irq_pin;
 
+		/*
+		 * Since there is only one interrupt available to
+		 * CardBus devices, all devices downstream of this
+		 * device must be using this IRQ.
+		 */
 		pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq_pin);
 		if (irq_pin) {
 			dev->irq = irq;
 			pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq);
 		}
 
+		/*
+		 * Some controllers transfer very slowly with 0 CLS.
+		 * Configure it.  This may fail as CLS configuration
+		 * is mandatory only for MWI.
+		 */
+		pci_set_cacheline_size(dev);
+
 		if (dev->subordinate)
-			cardbus_assign_irqs(dev->subordinate, irq);
+			cardbus_config_irq_and_cls(dev->subordinate, irq);
 	}
 }
 
@@ -228,7 +235,7 @@ int __ref cb_alloc(struct pcmcia_socket * s)
 	 */
 	pci_bus_size_bridges(bus);
 	pci_bus_assign_resources(bus);
-	cardbus_assign_irqs(bus, s->pci_irq);
+	cardbus_config_irq_and_cls(bus, s->pci_irq);
 
 	/* socket specific tune function */
 	if (s->tune_bridge)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 72698d8..e1a1aa6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -688,6 +688,7 @@ void pci_disable_device(struct pci_dev *dev);
 void pci_set_master(struct pci_dev *dev);
 void pci_clear_master(struct pci_dev *dev);
 int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state);
+int pci_set_cacheline_size(struct pci_dev *dev);
 #define HAVE_PCI_SET_MWI
 int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);

Re: [RFC PATCH] pccard: configure CLS on attach

[Matthew Wilcox]

On Wed, May 27, 2009 at 10:32:45PM +0900, Tejun Heo wrote:
> THIS IS A RFC PATCH, SO NO SOB.  PLEASE DON'T APPLY YET.

This breaks CONFIG_PPC64, fwiw.  We'll want to stub out
pci_set_cacheline_size() for the PCI_DISABLE_MWI case too.

I don't know what PPC machines have Cardbus slots, presumably some
Macs do.  I don't know whether firmware takes care of configuring the
Cacheline Size register for Cardbus hotplug or not.  So we may want to
include pci_set_cacheline_size() in the !MWI build, or not.  Ben, Paul?

> towerlexa, can you please test this patch?
> 
> Cc: Daniel Ritz <daniel.ritz@gmx.ch>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Greg KH <greg@kroah.com>
> Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Cc: towerlexa@gmx.de
> ---
>  drivers/pci/pci.c        |    3 +--
>  drivers/pcmcia/cardbus.c |   23 +++++++++++++++--------
>  include/linux/pci.h      |    1 +
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1a91bf9..eafbe01 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1860,8 +1860,7 @@ u8 pci_cache_line_size = PCI_CACHE_LINE_BYTES / 4;
>   *
>   * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
>   */
> -static int
> -pci_set_cacheline_size(struct pci_dev *dev)
> +int pci_set_cacheline_size(struct pci_dev *dev)
>  {
>  	u8 cacheline_size;
>  
> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
> index db77e1f..98789c0 100644
> --- a/drivers/pcmcia/cardbus.c
> +++ b/drivers/pcmcia/cardbus.c
> @@ -184,26 +184,33 @@ fail:
>      
>  =====================================================================*/
>  
> -/*
> - * Since there is only one interrupt available to CardBus
> - * devices, all devices downstream of this device must
> - * be using this IRQ.
> - */
> -static void cardbus_assign_irqs(struct pci_bus *bus, int irq)
> +static void cardbus_config_irq_and_cls(struct pci_bus *bus, int irq)
>  {
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		u8 irq_pin;
>  
> +		/*
> +		 * Since there is only one interrupt available to
> +		 * CardBus devices, all devices downstream of this
> +		 * device must be using this IRQ.
> +		 */
>  		pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq_pin);
>  		if (irq_pin) {
>  			dev->irq = irq;
>  			pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq);
>  		}
>  
> +		/*
> +		 * Some controllers transfer very slowly with 0 CLS.
> +		 * Configure it.  This may fail as CLS configuration
> +		 * is mandatory only for MWI.
> +		 */
> +		pci_set_cacheline_size(dev);
> +
>  		if (dev->subordinate)
> -			cardbus_assign_irqs(dev->subordinate, irq);
> +			cardbus_config_irq_and_cls(dev->subordinate, irq);
>  	}
>  }
>  
> @@ -228,7 +235,7 @@ int __ref cb_alloc(struct pcmcia_socket * s)
>  	 */
>  	pci_bus_size_bridges(bus);
>  	pci_bus_assign_resources(bus);
> -	cardbus_assign_irqs(bus, s->pci_irq);
> +	cardbus_config_irq_and_cls(bus, s->pci_irq);
>  
>  	/* socket specific tune function */
>  	if (s->tune_bridge)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 72698d8..e1a1aa6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -688,6 +688,7 @@ void pci_disable_device(struct pci_dev *dev);
>  void pci_set_master(struct pci_dev *dev);
>  void pci_clear_master(struct pci_dev *dev);
>  int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state);
> +int pci_set_cacheline_size(struct pci_dev *dev);
>  #define HAVE_PCI_SET_MWI
>  int __must_check pci_set_mwi(struct pci_dev *dev);
>  int pci_try_set_mwi(struct pci_dev *dev);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC PATCH] pccard: configure CLS on attach

[Tejun Heo]

Hello,

Matthew Wilcox wrote:
> On Wed, May 27, 2009 at 10:32:45PM +0900, Tejun Heo wrote:
>> THIS IS A RFC PATCH, SO NO SOB.  PLEASE DON'T APPLY YET.
> 
> This breaks CONFIG_PPC64, fwiw.  We'll want to stub out
> pci_set_cacheline_size() for the PCI_DISABLE_MWI case too.

Right, thanks for spotting it.

> I don't know what PPC machines have Cardbus slots, presumably some
> Macs do.  I don't know whether firmware takes care of configuring the
> Cacheline Size register for Cardbus hotplug or not.  So we may want to
> include pci_set_cacheline_size() in the !MWI build, or not.  Ben, Paul?

ppc64 is also missing PCI_CACHE_LINE_SIZE so pci_set_cacheline_size()
can't be built as-is.  BTW, on x86, pci_cache_line_size isn't
configured like other pci devices on many machines, which doesn't harm
correctness but still...  CLS being the same for all devices coming
down from the same root bridge, maybe we can do away with the current
logic and just take it from the upstream pci bridge?

Thanks.

-- 
tejun

Re: [RFC PATCH] pccard: configure CLS on attach

[Benjamin Herrenschmidt]

On Thu, 2009-05-28 at 08:11 +0900, Tejun Heo wrote:
> Hello,
> 
> Matthew Wilcox wrote:
> > On Wed, May 27, 2009 at 10:32:45PM +0900, Tejun Heo wrote:
> >> THIS IS A RFC PATCH, SO NO SOB.  PLEASE DON'T APPLY YET.
> > 
> > This breaks CONFIG_PPC64, fwiw.  We'll want to stub out
> > pci_set_cacheline_size() for the PCI_DISABLE_MWI case too.
> 
> Right, thanks for spotting it.
> 
> > I don't know what PPC machines have Cardbus slots, presumably some
> > Macs do.  I don't know whether firmware takes care of configuring the
> > Cacheline Size register for Cardbus hotplug or not.  So we may want to
> > include pci_set_cacheline_size() in the !MWI build, or not.  Ben, Paul?

Right, 32-bit Mac laptops mostly, maybe embedded stuff too. On these we
definitely want to configure stuff properly from the kernel.

> ppc64 is also missing PCI_CACHE_LINE_SIZE so pci_set_cacheline_size()
> can't be built as-is.

Well, the PCI cache line size would be a runtime thing. There are some
"issues" though on some HT platforms that I don't completely remember,
it really all depends on what the machine actually is.

So I'll need to have a look at the actual patch set to figure out
how we want to deal with it.

>   BTW, on x86, pci_cache_line_size isn't
> configured like other pci devices on many machines, which doesn't harm
> correctness but still...  CLS being the same for all devices coming
> down from the same root bridge, maybe we can do away with the current
> logic and just take it from the upstream pci bridge?

Cheers,
Ben.

Re: Who's responsible for configuring CLS on a cardbus device?

[Grant Grundler]

On Tue, May 26, 2009 at 02:23:00PM +0100, Alan Cox wrote:
...
> > This is solvable by simply setting CLS to the correct value but who's
> > job is it?  For non-hotplug devices, this is configured by the BIOS
> > (at least on PC), so for hotplug devices I think falls on the lap of
> > the PCI code but I'm not sure.  If this is something which the
> > sata_sil driver should be responsible for, is there an established way
> > to determine the proper CLS value?
> 
> Currently its handled by pci_set_mwi() but there isn't actually a more
> direct way to do this.

There isn't for the drivers because BIOS is supposed to set
PCI_CACHE_LINE_SIZE.

If the BIOS isn't setting PCI_CACHE_LINE_SIZE, the arch specific
pci support should be checking PCI_CACHE_LINE_SIZE and/or setting
it in pcibios_set_master().

hth,
grant

Re: [RFC PATCH] pccard: configure CLS on attach

[Axel Birndt]

Hello Tejun,

i'm sorry, but i don't know how to apply this patch. I'am not a
developer, and so i don't  know what i should do.

If it make sense (in your opinion) i would try this, but maybe someone
could me help please? (of course its not a problem to do the three-step:
configure, make, make install... but after this i don't know to remove
the installed files)

So, Maybe someone could build me an deb-package for my Kubuntu-System???

---------------
Linux UBUNTUNB 2.6.24-24-generic #1 SMP Wed Apr 15 15:54:25 UTC 2009
i686 GNU/Linux

....
cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=8.04
DISTRIB_CODENAME=hardy
DISTRIB_DESCRIPTION="Ubuntu 8.04.2"
------------------

Thanks and regards
Axel



Tejun Heo schrieb:
> 
> THIS IS A RFC PATCH, SO NO SOB.  PLEASE DON'T APPLY YET.
> 
> towerlexa, can you please test this patch?

> ---
>  drivers/pci/pci.c        |    3 +--
>  drivers/pcmcia/cardbus.c |   23 +++++++++++++++--------
>  include/linux/pci.h      |    1 +
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1a91bf9..eafbe01 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1860,8 +1860,7 @@ u8 pci_cache_line_size = PCI_CACHE_LINE_BYTES / 4;
>   *
>   * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
>   */
> -static int
> -pci_set_cacheline_size(struct pci_dev *dev)
> +int pci_set_cacheline_size(struct pci_dev *dev)
>  {
>  	u8 cacheline_size;
>  
> diff --git a/drivers/pcmcia/cardbus.c b/drivers/pcmcia/cardbus.c
> index db77e1f..98789c0 100644
> --- a/drivers/pcmcia/cardbus.c
> +++ b/drivers/pcmcia/cardbus.c
> @@ -184,26 +184,33 @@ fail:
>      
>  =====================================================================*/
>  
> -/*
> - * Since there is only one interrupt available to CardBus
> - * devices, all devices downstream of this device must
> - * be using this IRQ.
> - */
> -static void cardbus_assign_irqs(struct pci_bus *bus, int irq)
> +static void cardbus_config_irq_and_cls(struct pci_bus *bus, int irq)
>  {
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		u8 irq_pin;
>  
> +		/*
> +		 * Since there is only one interrupt available to
> +		 * CardBus devices, all devices downstream of this
> +		 * device must be using this IRQ.
> +		 */
>  		pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &irq_pin);
>  		if (irq_pin) {
>  			dev->irq = irq;
>  			pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq);
>  		}
>  
> +		/*
> +		 * Some controllers transfer very slowly with 0 CLS.
> +		 * Configure it.  This may fail as CLS configuration
> +		 * is mandatory only for MWI.
> +		 */
> +		pci_set_cacheline_size(dev);
> +
>  		if (dev->subordinate)
> -			cardbus_assign_irqs(dev->subordinate, irq);
> +			cardbus_config_irq_and_cls(dev->subordinate, irq);
>  	}
>  }
>  
> @@ -228,7 +235,7 @@ int __ref cb_alloc(struct pcmcia_socket * s)
>  	 */
>  	pci_bus_size_bridges(bus);
>  	pci_bus_assign_resources(bus);
> -	cardbus_assign_irqs(bus, s->pci_irq);
> +	cardbus_config_irq_and_cls(bus, s->pci_irq);
>  
>  	/* socket specific tune function */
>  	if (s->tune_bridge)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 72698d8..e1a1aa6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -688,6 +688,7 @@ void pci_disable_device(struct pci_dev *dev);
>  void pci_set_master(struct pci_dev *dev);
>  void pci_clear_master(struct pci_dev *dev);
>  int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state);
> +int pci_set_cacheline_size(struct pci_dev *dev);
>  #define HAVE_PCI_SET_MWI
>  int __must_check pci_set_mwi(struct pci_dev *dev);
>  int pci_try_set_mwi(struct pci_dev *dev);
>