RE: [PATCH v6] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, November 05, 2015 8:29 AM
> 
> From: Sowmini Varadhan 
> Date: Thu, 5 Nov 2015 11:28:31 -0500
> 
> > On (11/05/15 11:05), David Miller wrote:
> >> From: David Miller 
> >> Date: Thu, 05 Nov 2015 10:31:26 -0500 (EST)
> >>
> >> > I'll see if I can cook something up.
> >>
> >> How does this look?
> >
> > Looks good to me,
> >
> > Do you want me to respin patch v7 with this? Or update ixgbe/i40e to
> use
> > this later, after this goes in?
> 
> The intention is to let your patch go in as-is, then try and update
> ixgbe/i40e later in net-next or similar.

That works for us as well - thanks!
sln


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v6] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: David Miller [mailto:da...@davemloft.net]
> Sent: Thursday, November 05, 2015 8:29 AM
> 
> From: Sowmini Varadhan 
> Date: Thu, 5 Nov 2015 11:28:31 -0500
> 
> > On (11/05/15 11:05), David Miller wrote:
> >> From: David Miller 
> >> Date: Thu, 05 Nov 2015 10:31:26 -0500 (EST)
> >>
> >> > I'll see if I can cook something up.
> >>
> >> How does this look?
> >
> > Looks good to me,
> >
> > Do you want me to respin patch v7 with this? Or update ixgbe/i40e to
> use
> > this later, after this goes in?
> 
> The intention is to let your patch go in as-is, then try and update
> ixgbe/i40e later in net-next or similar.

That works for us as well - thanks!
sln


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v6] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Wednesday, November 04, 2015 3:21 PM
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.
> 
> In the case of the i40e there is an assumption that the default mac
> address has already been set up as the primary mac filter on probe,
> so if this filter is obtained from the Open Firmware or IDPROM, an
> explicit write is needed via i40e_aq_mac_address_write() and
> i40e_aq_add_macvlan() invocation.
> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2, v3: Andy Shevchenko comments
> v4: Shannon Nelson review: explicitly set up mac filters before
> register_netdev
> v5: Shannon Nelson code style comments
> v6: Shannon Nelson code style comments
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c |   83
> ++-
>  1 files changed, 82 insertions(+), 1 deletions(-)
> 

Acked-by: Shannon Nelson 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Wednesday, November 04, 2015 11:40 AM
> 
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.
> 
> In the case of the i40e there is an assumption that the default mac
> address has already been set up as the primary mac filter on probe,
> so if this filter is obtained from the Open Firmware or IDPROM, an
> explicit write is needed via i40e_aq_mac_address_write() and
> i40e_aq_add_macvlan() invocation.
> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2, v3: Andy Shevchenko comments
> v4: Shannon Nelson review: explicitly set up mac filters before
> register_netdev
> v5: Shannon Nelson code style comments
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c |   84
> ++-
>  1 files changed, 83 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b825f97..a3883cf 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -24,6 +24,15 @@
>   *
> 
> **
> /
> 
> +#include 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#endif
> +
>  /* Local includes */
>  #include "i40e.h"
>  #include "i40e_diag.h"
> @@ -9213,6 +9222,44 @@ static struct i40e_vsi
> *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>  }
> 
>  /**
> + * i40e_macaddr_init - explicitly write the mac address filters. This
> + * is needed when the macaddr has been obtained by other means than
> + * the default, e.g., from Open Firmware or IDPROM.

Note that this should be a simple single line, function name and short summary; 
anything more detailed goes into a description after the variables.


[...]

> 
>  /**
> + * i40e_get_platform_mac_addr - get mac address from Open Firmware
> + * or IDPROM if supported by the platform

Again, single line.

Thanks for your work on this, Sowmini.  If you can do a quick repost with these 
little function header comment bits tweaked, I'm willing to ACK this patch and 
I think we'll be ready for Jeff to include it into his tree.

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Wednesday, November 04, 2015 11:59 AM
> 
> On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
>  wrote:
> >
> > This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> > address in Open Firmware or IDPROM").

[...]

> > +   }
> > +
> > +   memset(, 0, sizeof(element));
> > +   ether_addr_copy(element.mac_addr, macaddr);
> > +   element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
> > +   ret = i40e_aq_add_macvlan(>back->hw, vsi->seid, ,
> 1, NULL);
> > +   aq_err = vsi->back->hw.aq.asq_last_status;
> 
> Do you really need a separate variable (aq_err)?

These are two separate error values that we're tracking - one from the 
communication between the driver and the firmware (aq_err) and one from the 
driver activity.  Sometimes there may be an AQ error that we want to report, 
but it might not actually be a driver error.  Alternatively, there are times 
when the AQ error needs to get interpreted different ways depending on which 
task the driver is performing.  Lastly, the AQ error gives us more detail on 
whatever the transaction error may have been which gives us more useful debug 
info.

sln

RE: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Wednesday, November 04, 2015 11:40 AM
> 
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.
> 
> In the case of the i40e there is an assumption that the default mac
> address has already been set up as the primary mac filter on probe,
> so if this filter is obtained from the Open Firmware or IDPROM, an
> explicit write is needed via i40e_aq_mac_address_write() and
> i40e_aq_add_macvlan() invocation.
> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2, v3: Andy Shevchenko comments
> v4: Shannon Nelson review: explicitly set up mac filters before
> register_netdev
> v5: Shannon Nelson code style comments
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c |   84
> ++-
>  1 files changed, 83 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b825f97..a3883cf 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -24,6 +24,15 @@
>   *
> 
> **
> /
> 
> +#include 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#endif
> +
>  /* Local includes */
>  #include "i40e.h"
>  #include "i40e_diag.h"
> @@ -9213,6 +9222,44 @@ static struct i40e_vsi
> *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>  }
> 
>  /**
> + * i40e_macaddr_init - explicitly write the mac address filters. This
> + * is needed when the macaddr has been obtained by other means than
> + * the default, e.g., from Open Firmware or IDPROM.

Note that this should be a simple single line, function name and short summary; 
anything more detailed goes into a description after the variables.


[...]

> 
>  /**
> + * i40e_get_platform_mac_addr - get mac address from Open Firmware
> + * or IDPROM if supported by the platform

Again, single line.

Thanks for your work on this, Sowmini.  If you can do a quick repost with these 
little function header comment bits tweaked, I'm willing to ACK this patch and 
I think we'll be ready for Jeff to include it into his tree.

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Wednesday, November 04, 2015 11:59 AM
> 
> On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
>  wrote:
> >
> > This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> > address in Open Firmware or IDPROM").

[...]

> > +   }
> > +
> > +   memset(, 0, sizeof(element));
> > +   ether_addr_copy(element.mac_addr, macaddr);
> > +   element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
> > +   ret = i40e_aq_add_macvlan(>back->hw, vsi->seid, ,
> 1, NULL);
> > +   aq_err = vsi->back->hw.aq.asq_last_status;
> 
> Do you really need a separate variable (aq_err)?

These are two separate error values that we're tracking - one from the 
communication between the driver and the firmware (aq_err) and one from the 
driver activity.  Sometimes there may be an AQ error that we want to report, 
but it might not actually be a driver error.  Alternatively, there are times 
when the AQ error needs to get interpreted different ways depending on which 
task the driver is performing.  Lastly, the AQ error gives us more detail on 
whatever the transaction error may have been which gives us more useful debug 
info.

sln

RE: [PATCH v6] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Wednesday, November 04, 2015 3:21 PM
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.
> 
> In the case of the i40e there is an assumption that the default mac
> address has already been set up as the primary mac filter on probe,
> so if this filter is obtained from the Open Firmware or IDPROM, an
> explicit write is needed via i40e_aq_mac_address_write() and
> i40e_aq_add_macvlan() invocation.
> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2, v3: Andy Shevchenko comments
> v4: Shannon Nelson review: explicitly set up mac filters before
> register_netdev
> v5: Shannon Nelson code style comments
> v6: Shannon Nelson code style comments
> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c |   83
> ++-
>  1 files changed, 82 insertions(+), 1 deletions(-)
> 

Acked-by: Shannon Nelson 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v4 RFC net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Sunday, November 01, 2015 8:25 AM
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.
> 
> In the case of the i40e there is an assumption that the default mac
> address has already been set up as the primary mac filter on probe,
> so if this filter is obtained from the Open Firmware or IDPROM, an
> explicit write is needed via i40e_aq_mac_address_write() and
> i40e_aq_add_macvlan() invocation.
> 
> Reviewers: please check if invoking i40e_macaddr_init() on
> platforms that use the default mac address (i.e., when it is not from
> OF or idprom) will cause harm, and if it is necessary/possible to
> move this invocation to an earlier point in i40e_probe().
> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2, v3: Andy Shevchenko comments
> v4:

Thanks, Sowmini, this looks reasonable to me, aside from a few simple stylistic 
comments below.

sln

> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c |   53
> ++-
>  1 files changed, 52 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b825f97..3c81c0c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -24,6 +24,15 @@
>   *
> 
> **
> /
> 
> +#include 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#endif
> +
>  /* Local includes */
>  #include "i40e.h"
>  #include "i40e_diag.h"
> @@ -9212,6 +9221,25 @@ static struct i40e_vsi
> *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>   return NULL;
>  }
> 

Please add a function comment in the style of the rest of the code.

> +static int i40e_macaddr_init( struct i40e_vsi *vsi, u8 *macaddr)

No space between '(' and 'struct'

> +{
> + int ret;
> + struct i40e_aqc_add_macvlan_element_data element;
> +
> + ret = i40e_aq_mac_address_write(>back->hw,
> + I40E_AQC_WRITE_TYPE_LAA_WOL,
> + macaddr, NULL);
> + if (ret)
> + return -EADDRNOTAVAIL;

Please report the AQ error here as we do elsewhere in the code

> +
> + memset(, 0, sizeof(element));
> + ether_addr_copy(element.mac_addr, macaddr);
> + element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
> + i40e_aq_add_macvlan(>back->hw, vsi->seid, , 1, NULL);

Please check for and report any AQ error returned from this call as we do 
elsewhere in the code.

> +
> + return ret;
> +}
> +
>  /**
>   * i40e_vsi_setup - Set up a VSI by a given type
>   * @pf: board private structure
> @@ -9341,6 +9369,9 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf,
> u8 type,
>   ret = i40e_config_netdev(vsi);
>   if (ret)
>   goto err_netdev;
> + ret = i40e_macaddr_init(vsi, pf->hw.mac.addr);
> + if (ret)
> + goto err_netdev;
>   ret = register_netdev(vsi->netdev);
>   if (ret)
>   goto err_netdev;
> @@ -10162,6 +10193,24 @@ static void i40e_print_features(struct i40e_pf
> *pf)
>   kfree(string);
>  }
> 

Function comment needed

> +static int i40e_get_platform_mac_addr(struct pci_dev *pdev, u8 *mac_addr)
> +{
> + struct device_node *dp = pci_device_to_OF_node(pdev);
> + const unsigned char *addr;
> +
> + addr = of_get_mac_address(dp);
> + if (addr) {
> + ether_addr_copy(mac_addr, addr);
> + return 0;
> + }
> +#ifdef CONFIG_SPARC
> + ether_addr_copy(mac_addr, idprom->id_ethaddr);
> + return 0;
> +#else
> + return -EINVAL;
> +#endif /* CONFIG_SPARC */
> +}
> +
>  /**
>   * i40e_probe - Device initialization routine
>   * @pdev: PCI device information struct
> @@ -10360,7 +10409,9 @@ static int i40e_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>   i40e_aq_stop_lldp(hw, true, NULL);
>   }
> 
> - i40e_get_mac_addr(hw, hw->mac.addr);
> + err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
> + if (err)
> + i40e_get_mac_addr(hw, hw->mac.addr);
>   if (!is_valid_ether_addr(hw->mac.addr)) {
>   dev_info(>dev, "invalid MAC address %pM\n", hw-
> >mac.addr);
>   err = -EIO;
> --
> 1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v4 RFC net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Sunday, November 01, 2015 8:25 AM
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.
> 
> In the case of the i40e there is an assumption that the default mac
> address has already been set up as the primary mac filter on probe,
> so if this filter is obtained from the Open Firmware or IDPROM, an
> explicit write is needed via i40e_aq_mac_address_write() and
> i40e_aq_add_macvlan() invocation.
> 
> Reviewers: please check if invoking i40e_macaddr_init() on
> platforms that use the default mac address (i.e., when it is not from
> OF or idprom) will cause harm, and if it is necessary/possible to
> move this invocation to an earlier point in i40e_probe().
> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2, v3: Andy Shevchenko comments
> v4:

Thanks, Sowmini, this looks reasonable to me, aside from a few simple stylistic 
comments below.

sln

> 
>  drivers/net/ethernet/intel/i40e/i40e_main.c |   53
> ++-
>  1 files changed, 52 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b825f97..3c81c0c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -24,6 +24,15 @@
>   *
> 
> **
> /
> 
> +#include 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#endif
> +
>  /* Local includes */
>  #include "i40e.h"
>  #include "i40e_diag.h"
> @@ -9212,6 +9221,25 @@ static struct i40e_vsi
> *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>   return NULL;
>  }
> 

Please add a function comment in the style of the rest of the code.

> +static int i40e_macaddr_init( struct i40e_vsi *vsi, u8 *macaddr)

No space between '(' and 'struct'

> +{
> + int ret;
> + struct i40e_aqc_add_macvlan_element_data element;
> +
> + ret = i40e_aq_mac_address_write(>back->hw,
> + I40E_AQC_WRITE_TYPE_LAA_WOL,
> + macaddr, NULL);
> + if (ret)
> + return -EADDRNOTAVAIL;

Please report the AQ error here as we do elsewhere in the code

> +
> + memset(, 0, sizeof(element));
> + ether_addr_copy(element.mac_addr, macaddr);
> + element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
> + i40e_aq_add_macvlan(>back->hw, vsi->seid, , 1, NULL);

Please check for and report any AQ error returned from this call as we do 
elsewhere in the code.

> +
> + return ret;
> +}
> +
>  /**
>   * i40e_vsi_setup - Set up a VSI by a given type
>   * @pf: board private structure
> @@ -9341,6 +9369,9 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf,
> u8 type,
>   ret = i40e_config_netdev(vsi);
>   if (ret)
>   goto err_netdev;
> + ret = i40e_macaddr_init(vsi, pf->hw.mac.addr);
> + if (ret)
> + goto err_netdev;
>   ret = register_netdev(vsi->netdev);
>   if (ret)
>   goto err_netdev;
> @@ -10162,6 +10193,24 @@ static void i40e_print_features(struct i40e_pf
> *pf)
>   kfree(string);
>  }
> 

Function comment needed

> +static int i40e_get_platform_mac_addr(struct pci_dev *pdev, u8 *mac_addr)
> +{
> + struct device_node *dp = pci_device_to_OF_node(pdev);
> + const unsigned char *addr;
> +
> + addr = of_get_mac_address(dp);
> + if (addr) {
> + ether_addr_copy(mac_addr, addr);
> + return 0;
> + }
> +#ifdef CONFIG_SPARC
> + ether_addr_copy(mac_addr, idprom->id_ethaddr);
> + return 0;
> +#else
> + return -EINVAL;
> +#endif /* CONFIG_SPARC */
> +}
> +
>  /**
>   * i40e_probe - Device initialization routine
>   * @pdev: PCI device information struct
> @@ -10360,7 +10409,9 @@ static int i40e_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>   i40e_aq_stop_lldp(hw, true, NULL);
>   }
> 
> - i40e_get_mac_addr(hw, hw->mac.addr);
> + err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
> + if (err)
> + i40e_get_mac_addr(hw, hw->mac.addr);
>   if (!is_valid_ether_addr(hw->mac.addr)) {
>   dev_info(>dev, "invalid MAC address %pM\n", hw-
> >mac.addr);
>   err = -EIO;
> --
> 1.7.1

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

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Sunday, November 01, 2015 8:24 AM
> 

[...]

> So I figured out why it all "seemed to work" - my test env had another
> obscure init process that was marking the link promiscuous.  I guess
> that was having the side-effect of somehow setting the filters above.
> 
> But looks like there's more to getting this right than just calling
> i40e_aq_mac_address_write() - I think it also needs a
> i40e_aq_add_macvlan().
> 
> I was able to get this to work by calling a the core part of
> i40e_set_mac just before register_netdev. In my patch (RFC patch
> in a separate thread - please review) I now have this sequence in
> i40e_probe
> 
>   err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
>   if (err)
>   i40e_get_mac_addr(hw, hw->mac.addr);
>:
>   i40e_setup_pf_switch(..);
> 
> And the resulting i40e_vsi_setup() from i40e_setup_pf_switch()
> will end up doing the right thing by invoking the guts of
> i40e_set_mac(), which is basically the  sequence:
>   i40e_aq_mac_address_write()
>   i40e_aq_add_macvlan()
> 
> I dont know if it is necessary/possible/important to set up the
> filters sooner in the sequence- the add_macvlan needs an "seid",
> and I could not tell when (in the ":" code above) the right seid
> can be found.
> 
> Please review the RFC patch I'll be sending shortly.
> 
> --Sowmini

Yeah, because of the underlying HW we need to manage, and the fact that we 
can't ask it for the filters it knows, it becomes a bit convoluted.  To manage 
and replay the filter lists, we use the i40e_{add,del}_filter() routines on the 
individual VSI filter lists.  I'm thinking you're on the right track, but we 
may not want to bypass the VSI's filter list.

My brain's not in gear this weekend so I'll review the patch later.  In the 
meantime, be sure to test what happens over a reset, such as what happens when 
the MTU is changed.  This will make sure that the replay of mac and vlan 
filters happens correctly.  You'll want to test this with and without vlans.

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Sunday, November 01, 2015 8:24 AM
> 

[...]

> So I figured out why it all "seemed to work" - my test env had another
> obscure init process that was marking the link promiscuous.  I guess
> that was having the side-effect of somehow setting the filters above.
> 
> But looks like there's more to getting this right than just calling
> i40e_aq_mac_address_write() - I think it also needs a
> i40e_aq_add_macvlan().
> 
> I was able to get this to work by calling a the core part of
> i40e_set_mac just before register_netdev. In my patch (RFC patch
> in a separate thread - please review) I now have this sequence in
> i40e_probe
> 
>   err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
>   if (err)
>   i40e_get_mac_addr(hw, hw->mac.addr);
>:
>   i40e_setup_pf_switch(..);
> 
> And the resulting i40e_vsi_setup() from i40e_setup_pf_switch()
> will end up doing the right thing by invoking the guts of
> i40e_set_mac(), which is basically the  sequence:
>   i40e_aq_mac_address_write()
>   i40e_aq_add_macvlan()
> 
> I dont know if it is necessary/possible/important to set up the
> filters sooner in the sequence- the add_macvlan needs an "seid",
> and I could not tell when (in the ":" code above) the right seid
> can be found.
> 
> Please review the RFC patch I'll be sending shortly.
> 
> --Sowmini

Yeah, because of the underlying HW we need to manage, and the fact that we 
can't ask it for the filters it knows, it becomes a bit convoluted.  To manage 
and replay the filter lists, we use the i40e_{add,del}_filter() routines on the 
individual VSI filter lists.  I'm thinking you're on the right track, but we 
may not want to bypass the VSI's filter list.

My brain's not in gear this weekend so I'll review the patch later.  In the 
meantime, be sure to test what happens over a reset, such as what happens when 
the MTU is changed.  This will make sure that the replay of mac and vlan 
filters happens correctly.  You'll want to test this with and without vlans.

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Friday, October 30, 2015 12:25 PM
> 
> On (10/30/15 18:57), Nelson, Shannon wrote:
> > > >
> > > > Going along with this being the equivalent of the ixgbe patch, I'd
> > > > prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
> > > > In the design of our drivers, the common file is essentially a device
> > > > specific layer, and the OS and platform related stuff should stay in
> > > > i40e_main.c.  That would also take care of one of the include file nits
> > > > that Andy mentioned.
>:
> > I'm not sure about any deeper wisdom, but the HW/FW model that this
> > device uses is rather different from our previous devices, so our SW
> > abstractions are a little different from ixgbe and igb.
> 
> Ok, in that case I can make i40e_main.c to do something like
> 
> static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
>:
>if (i40e_get_platform_mac_addr(..) != I40E_SUCCESS)
>   i40e_get_mac_addr(..);
>:
> }
> 
> and i agree that doing this from i40e_main will simplify a lot of the
> other stuff around getting the pdev etc.

The more common idiom in our driver would be

err = i40e_get_platform_mac_addr(..);
if (err) {
...
}

> 
> [Discussion about ndo_set_mac_address..]
> 
> > In the cases in which I'm aware, the platform manufacturer normally will
> > burn a different mac-address into the device's eeprom at manufacturing
> > time if they want something other than what came from Intel.  I suppose a
> > Device Tree oriented platform could have a different address in the DT,
> > but somehow that needs to get communicated to the device driver so that
> > it can tell the HW.
> >
> > My question to you remains: does this ndo_set_mac_address happen from
> > the stack above the driver or not?
> 
> I'm probably even less clued in to the DT arch than you in this case,
> so input from the intel experts would be useful here.

This worries me - if you're not clued into the DT architecture, why are you 
making these changes?  Have you tested this beyond a compile?  Do you have a DT 
model to try this against?

> 
> But both in this case, and for the ixgbe template on which I tried
> to model this, the OF/idprom probing happens from the ->probe when the
> driver comes up, and ndo_set_mac_address is not involved.
> 
> I dont know if it is easier (or even feasible to do this from ->probe)
> to just call the i40e_set_mac to make sure all the state-bits in
> the driver are correctly set.
> 
> --Sowmini

In looking at a couple other drivers, I see the difference being that they 
typically are writing the primary mac filter on probe (and any other reset), 
whereas the i40e "knows" that the default mac address is already set up as the 
filter and doesn't bother with a redundant write.  If you want to add this Open 
Filter code, you'll need to arrange for this write to happen.  You can't call 
i40e_set_mac() to do it, but you can see the i40e_aq_mac_address_write() code 
there that is involved in updating the mac address as an example.  You probably 
will want to look at section 4.2.1.5.3 of the XL710 data sheet in order to know 
how to use i40e_aq_mac_address_write() for your situation.

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Friday, October 30, 2015 11:37 AM
> To: 
> Subject: Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or
> IDPROM
> 
> On (10/30/15 18:28), Nelson, Shannon wrote:
> >
> > Going along with this being the equivalent of the ixgbe patch, I'd
> > prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
> > In the design of our drivers, the common file is essentially a device
> > specific layer, and the OS and platform related stuff should stay in
> > i40e_main.c.  That would also take care of one of the include file nits
> > that Andy mentioned.
> 
> ok, and that was my initial instinct as well, except that I noticed
> that the existing i40e code tries interesting variations from the
> typical intel driver model by calling i40e_get_mac_addr() which lives in
> (for reasons not obvious to me) i40e_common.c
> 
> So I assumed there must be some other deeper wisdom at work here.

I'm not sure about any deeper wisdom, but the HW/FW model that this device uses 
is rather different from our previous devices, so our SW abstractions are a 
little different from ixgbe and igb.

> 
> > At the risk of flying my Device Tree ignorance for all to see, I have
> > a couple questions on how this is used.
> >
> > Since the mac address is specific to the individual device, how does it
> > get into the device tree?  I thought the device tree was compiled ahead
> > of time for the platform it is used on.  Is this a generic DT pattern
> > just in case some platform actually has the mac-address?  Or does the
> > device mac-address get saved into the DT on the first time through this
> > path so it can be found next time?
> >
> > If the Device Tree has a different mac address than the HW, when
> > does the kernel tell the HW to use a different mac address?  Does the
> > DT management eventually call the ndo_set_mac_address call so the HW
> > knows to use a different mac address?
> 
> yes, and here I was hoping for some feedback from the intel folks as
> well. Commit c762dff24c06 sets hw->mac.perm_addr. I dont know
> if there is some similar i40e state that needs to be set.
> 
> Please share insights.
> 
> --Sowmini

Forgive me if you're already up with this, I just want to be sure we're 
thinking along the same lines.

The HW has a mac address burned into its own eeprom which it will use by 
default.  The driver merely queries the HW to find out what that mac address is 
and report it to the OS so it can be registered correctly in the netdev.

If a different mac address is desired, perhaps from the user issuing an "ip 
link" or ifconfig command, the OS will call the netdev's ndo_set_mac_address to 
tell the driver.  In our case, the driver then tells the HW device to filter 
for the new mac address rather than the eeprom'd address.  This remains until a 
reset when the HW goes back to its eeprom'd address.

In the cases in which I'm aware, the platform manufacturer normally will burn a 
different mac-address into the device's eeprom at manufacturing time if they 
want something other than what came from Intel.  I suppose a Device Tree 
oriented platform could have a different address in the DT, but somehow that 
needs to get communicated to the device driver so that it can tell the HW.

My question to you remains: does this ndo_set_mac_address happen from the stack 
above the driver or not?

Thanks,
sln


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Friday, October 30, 2015 8:04 AM
> To: 
> Subject: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM
> 
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.

Going along with this being the equivalent of the ixgbe patch, I'd prefer the 
new code to be in i40e_main.c, rather than in i40e_common.c.  In the design of 
our drivers, the common file is essentially a device specific layer, and the OS 
and platform related stuff should stay in i40e_main.c.  That would also take 
care of one of the include file nits that Andy mentioned.


At the risk of flying my Device Tree ignorance for all to see, I have a couple 
questions on how this is used.

Since the mac address is specific to the individual device, how does it get 
into the device tree?  I thought the device tree was compiled ahead of time for 
the platform it is used on.  Is this a generic DT pattern just in case some 
platform actually has the mac-address?  Or does the device mac-address get 
saved into the DT on the first time through this path so it can be found next 
time?

If the Device Tree has a different mac address than the HW, when does the 
kernel tell the HW to use a different mac address?  Does the DT management 
eventually call the ndo_set_mac_address call so the HW knows to use a different 
mac address?

Thanks,
sln


> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2: andy shevchenko comments
> v3: more andy shevchenko comments
>  drivers/net/ethernet/intel/i40e/i40e_common.c |   30 
> +
>  1 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 2d74c6e..3edfe19 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -24,6 +24,14 @@
>   *
> 
> **/
> 
> +#include 
> +#include 
> +#include 
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#endif
> +#include "i40e.h"
>  #include "i40e_type.h"
>  #include "i40e_adminq.h"
>  #include "i40e_prototype.h"
> @@ -1008,6 +1016,25 @@ i40e_status i40e_aq_mac_address_write(struct i40e_hw
> *hw,
>   return status;
>  }
> 
> +static int i40e_get_platform_mac_addr(struct i40e_hw *hw, u8 *mac_addr)
> +{
> + struct i40e_pf *pf = hw->back;
> + struct device_node *dp = pci_device_to_OF_node(pf->pdev);
> + const unsigned char *addr;
> +
> + addr = of_get_mac_address(dp);
> + if (addr) {
> + ether_addr_copy(mac_addr, addr);
> + return 0;
> + }
> +
> +#ifdef CONFIG_SPARC
> + ether_addr_copy(mac_addr, idprom->id_ethaddr);
> + return 0;
> +#endif /* CONFIG_SPARC */
> + return 1;
> +}
> +
>  /**
>   * i40e_get_mac_addr - get MAC address
>   * @hw: pointer to the HW structure
> @@ -1021,6 +1048,9 @@ i40e_status i40e_get_mac_addr(struct i40e_hw *hw, u8
> *mac_addr)
>   i40e_status status;
>   u16 flags = 0;
> 
> + if (!i40e_get_platform_mac_addr(hw, mac_addr))
> + return I40E_SUCCESS;
> +
>   status = i40e_aq_mac_address_read(hw, , , NULL);
> 
>   if (flags & I40E_AQC_LAN_ADDR_VALID)
> --
> 1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Friday, October 30, 2015 12:25 PM
> 
> On (10/30/15 18:57), Nelson, Shannon wrote:
> > > >
> > > > Going along with this being the equivalent of the ixgbe patch, I'd
> > > > prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
> > > > In the design of our drivers, the common file is essentially a device
> > > > specific layer, and the OS and platform related stuff should stay in
> > > > i40e_main.c.  That would also take care of one of the include file nits
> > > > that Andy mentioned.
>:
> > I'm not sure about any deeper wisdom, but the HW/FW model that this
> > device uses is rather different from our previous devices, so our SW
> > abstractions are a little different from ixgbe and igb.
> 
> Ok, in that case I can make i40e_main.c to do something like
> 
> static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
>:
>if (i40e_get_platform_mac_addr(..) != I40E_SUCCESS)
>   i40e_get_mac_addr(..);
>:
> }
> 
> and i agree that doing this from i40e_main will simplify a lot of the
> other stuff around getting the pdev etc.

The more common idiom in our driver would be

err = i40e_get_platform_mac_addr(..);
if (err) {
...
}

> 
> [Discussion about ndo_set_mac_address..]
> 
> > In the cases in which I'm aware, the platform manufacturer normally will
> > burn a different mac-address into the device's eeprom at manufacturing
> > time if they want something other than what came from Intel.  I suppose a
> > Device Tree oriented platform could have a different address in the DT,
> > but somehow that needs to get communicated to the device driver so that
> > it can tell the HW.
> >
> > My question to you remains: does this ndo_set_mac_address happen from
> > the stack above the driver or not?
> 
> I'm probably even less clued in to the DT arch than you in this case,
> so input from the intel experts would be useful here.

This worries me - if you're not clued into the DT architecture, why are you 
making these changes?  Have you tested this beyond a compile?  Do you have a DT 
model to try this against?

> 
> But both in this case, and for the ixgbe template on which I tried
> to model this, the OF/idprom probing happens from the ->probe when the
> driver comes up, and ndo_set_mac_address is not involved.
> 
> I dont know if it is easier (or even feasible to do this from ->probe)
> to just call the i40e_set_mac to make sure all the state-bits in
> the driver are correctly set.
> 
> --Sowmini

In looking at a couple other drivers, I see the difference being that they 
typically are writing the primary mac filter on probe (and any other reset), 
whereas the i40e "knows" that the default mac address is already set up as the 
filter and doesn't bother with a redundant write.  If you want to add this Open 
Filter code, you'll need to arrange for this write to happen.  You can't call 
i40e_set_mac() to do it, but you can see the i40e_aq_mac_address_write() code 
there that is involved in updating the mac address as an example.  You probably 
will want to look at section 4.2.1.5.3 of the XL710 data sheet in order to know 
how to use i40e_aq_mac_address_write() for your situation.

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Friday, October 30, 2015 11:37 AM
> To: 
> Subject: Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or
> IDPROM
> 
> On (10/30/15 18:28), Nelson, Shannon wrote:
> >
> > Going along with this being the equivalent of the ixgbe patch, I'd
> > prefer the new code to be in i40e_main.c, rather than in i40e_common.c.
> > In the design of our drivers, the common file is essentially a device
> > specific layer, and the OS and platform related stuff should stay in
> > i40e_main.c.  That would also take care of one of the include file nits
> > that Andy mentioned.
> 
> ok, and that was my initial instinct as well, except that I noticed
> that the existing i40e code tries interesting variations from the
> typical intel driver model by calling i40e_get_mac_addr() which lives in
> (for reasons not obvious to me) i40e_common.c
> 
> So I assumed there must be some other deeper wisdom at work here.

I'm not sure about any deeper wisdom, but the HW/FW model that this device uses 
is rather different from our previous devices, so our SW abstractions are a 
little different from ixgbe and igb.

> 
> > At the risk of flying my Device Tree ignorance for all to see, I have
> > a couple questions on how this is used.
> >
> > Since the mac address is specific to the individual device, how does it
> > get into the device tree?  I thought the device tree was compiled ahead
> > of time for the platform it is used on.  Is this a generic DT pattern
> > just in case some platform actually has the mac-address?  Or does the
> > device mac-address get saved into the DT on the first time through this
> > path so it can be found next time?
> >
> > If the Device Tree has a different mac address than the HW, when
> > does the kernel tell the HW to use a different mac address?  Does the
> > DT management eventually call the ndo_set_mac_address call so the HW
> > knows to use a different mac address?
> 
> yes, and here I was hoping for some feedback from the intel folks as
> well. Commit c762dff24c06 sets hw->mac.perm_addr. I dont know
> if there is some similar i40e state that needs to be set.
> 
> Please share insights.
> 
> --Sowmini

Forgive me if you're already up with this, I just want to be sure we're 
thinking along the same lines.

The HW has a mac address burned into its own eeprom which it will use by 
default.  The driver merely queries the HW to find out what that mac address is 
and report it to the OS so it can be registered correctly in the netdev.

If a different mac address is desired, perhaps from the user issuing an "ip 
link" or ifconfig command, the OS will call the netdev's ndo_set_mac_address to 
tell the driver.  In our case, the driver then tells the HW device to filter 
for the new mac address rather than the eeprom'd address.  This remains until a 
reset when the HW goes back to its eeprom'd address.

In the cases in which I'm aware, the platform manufacturer normally will burn a 
different mac-address into the device's eeprom at manufacturing time if they 
want something other than what came from Intel.  I suppose a Device Tree 
oriented platform could have a different address in the DT, but somehow that 
needs to get communicated to the device driver so that it can tell the HW.

My question to you remains: does this ndo_set_mac_address happen from the stack 
above the driver or not?

Thanks,
sln


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -Original Message-
> From: Sowmini Varadhan [mailto:sowmini.varad...@oracle.com]
> Sent: Friday, October 30, 2015 8:04 AM
> To: 
> Subject: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM
> 
> 
> This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> address in Open Firmware or IDPROM").
> 
> As with that fix, attempt to look up the MAC address in Open Firmware
> on systems that support it, and use IDPROM on SPARC if no OF address
> is found.

Going along with this being the equivalent of the ixgbe patch, I'd prefer the 
new code to be in i40e_main.c, rather than in i40e_common.c.  In the design of 
our drivers, the common file is essentially a device specific layer, and the OS 
and platform related stuff should stay in i40e_main.c.  That would also take 
care of one of the include file nits that Andy mentioned.


At the risk of flying my Device Tree ignorance for all to see, I have a couple 
questions on how this is used.

Since the mac address is specific to the individual device, how does it get 
into the device tree?  I thought the device tree was compiled ahead of time for 
the platform it is used on.  Is this a generic DT pattern just in case some 
platform actually has the mac-address?  Or does the device mac-address get 
saved into the DT on the first time through this path so it can be found next 
time?

If the Device Tree has a different mac address than the HW, when does the 
kernel tell the HW to use a different mac address?  Does the DT management 
eventually call the ndo_set_mac_address call so the HW knows to use a different 
mac address?

Thanks,
sln


> 
> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> v2: andy shevchenko comments
> v3: more andy shevchenko comments
>  drivers/net/ethernet/intel/i40e/i40e_common.c |   30 
> +
>  1 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 2d74c6e..3edfe19 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -24,6 +24,14 @@
>   *
> 
> **/
> 
> +#include 
> +#include 
> +#include 
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#endif
> +#include "i40e.h"
>  #include "i40e_type.h"
>  #include "i40e_adminq.h"
>  #include "i40e_prototype.h"
> @@ -1008,6 +1016,25 @@ i40e_status i40e_aq_mac_address_write(struct i40e_hw
> *hw,
>   return status;
>  }
> 
> +static int i40e_get_platform_mac_addr(struct i40e_hw *hw, u8 *mac_addr)
> +{
> + struct i40e_pf *pf = hw->back;
> + struct device_node *dp = pci_device_to_OF_node(pf->pdev);
> + const unsigned char *addr;
> +
> + addr = of_get_mac_address(dp);
> + if (addr) {
> + ether_addr_copy(mac_addr, addr);
> + return 0;
> + }
> +
> +#ifdef CONFIG_SPARC
> + ether_addr_copy(mac_addr, idprom->id_ethaddr);
> + return 0;
> +#endif /* CONFIG_SPARC */
> + return 1;
> +}
> +
>  /**
>   * i40e_get_mac_addr - get MAC address
>   * @hw: pointer to the HW structure
> @@ -1021,6 +1048,9 @@ i40e_status i40e_get_mac_addr(struct i40e_hw *hw, u8
> *mac_addr)
>   i40e_status status;
>   u16 flags = 0;
> 
> + if (!i40e_get_platform_mac_addr(hw, mac_addr))
> + return I40E_SUCCESS;
> +
>   status = i40e_aq_mac_address_read(hw, , , NULL);
> 
>   if (flags & I40E_AQC_LAN_ADDR_VALID)
> --
> 1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] intel: i40e: fix confused code

[Nelson, Shannon]

> -Original Message-
> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> Sent: Saturday, October 17, 2015 1:58 PM
> Subject: [PATCH] intel: i40e: fix confused code
> 
> This code is pretty confused. The variable name 'bytes_not_copied'
> clearly indicates that the programmer knew the semantics of
> copy_{to,from}_user, but then the return value is checked for being
> negative and used as a -Exxx return value.
> 
> I'm not sure this is the proper fix, but at least we get rid of the
> dead code which pretended to check for access faults.
> 
> Signed-off-by: Rasmus Villemoes 
> ---

Acked-by: Shannon Nelson 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] intel: i40e: fix confused code

[Nelson, Shannon]

> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> Sent: Tuesday, October 20, 2015 12:22 AM
> 
> On Mon, Oct 19 2015, "Nelson, Shannon"  wrote:
> 
> >> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> >> Sent: Saturday, October 17, 2015 1:58 PM
> >> Subject: [PATCH] intel: i40e: fix confused code
> >>
> >> This code is pretty confused. The variable name 'bytes_not_copied'
> >> clearly indicates that the programmer knew the semantics of
> >> copy_{to,from}_user, but then the return value is checked for being
> >> negative and used as a -Exxx return value.
> >>
> >> I'm not sure this is the proper fix, but at least we get rid of the
> >> dead code which pretended to check for access faults.
> >>
> >> Signed-off-by: Rasmus Villemoes 
> >
> > I believe this patch is unnecessary: if the value is negative, then it
> > already is an error code giving some potentially useful information.
> > When I dig into the copy_to_user() code, I see in the comments for
> > put_user() that -EFAULT is the error being returned.
> 
> Thanks, this was precisely the kind of confusion I'm talking about:
> copy_{from,to}_user _never_ returns a negative value. It returns
> precisely what the very explicit variable name hints.
> 
> This is in contrast to the single-scalar functions get_user/put_user,
> which do return -EFAULT for error and 0 for success.
> 
> (See also lines 479-519 of Documentation/DocBook/kernel-hacking.tmpl).

I like the comment about the moronic interface for copy_to/from_user...

Yes, I see where I turned left instead of right.  This would be good to fix up.

sln
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] intel: i40e: fix confused code

[Nelson, Shannon]

> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> Sent: Tuesday, October 20, 2015 12:22 AM
> 
> On Mon, Oct 19 2015, "Nelson, Shannon" <shannon.nel...@intel.com> wrote:
> 
> >> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> >> Sent: Saturday, October 17, 2015 1:58 PM
> >> Subject: [PATCH] intel: i40e: fix confused code
> >>
> >> This code is pretty confused. The variable name 'bytes_not_copied'
> >> clearly indicates that the programmer knew the semantics of
> >> copy_{to,from}_user, but then the return value is checked for being
> >> negative and used as a -Exxx return value.
> >>
> >> I'm not sure this is the proper fix, but at least we get rid of the
> >> dead code which pretended to check for access faults.
> >>
> >> Signed-off-by: Rasmus Villemoes <li...@rasmusvillemoes.dk>
> >
> > I believe this patch is unnecessary: if the value is negative, then it
> > already is an error code giving some potentially useful information.
> > When I dig into the copy_to_user() code, I see in the comments for
> > put_user() that -EFAULT is the error being returned.
> 
> Thanks, this was precisely the kind of confusion I'm talking about:
> copy_{from,to}_user _never_ returns a negative value. It returns
> precisely what the very explicit variable name hints.
> 
> This is in contrast to the single-scalar functions get_user/put_user,
> which do return -EFAULT for error and 0 for success.
> 
> (See also lines 479-519 of Documentation/DocBook/kernel-hacking.tmpl).

I like the comment about the moronic interface for copy_to/from_user...

Yes, I see where I turned left instead of right.  This would be good to fix up.

sln
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] intel: i40e: fix confused code

[Nelson, Shannon]

> -Original Message-
> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> Sent: Saturday, October 17, 2015 1:58 PM
> Subject: [PATCH] intel: i40e: fix confused code
> 
> This code is pretty confused. The variable name 'bytes_not_copied'
> clearly indicates that the programmer knew the semantics of
> copy_{to,from}_user, but then the return value is checked for being
> negative and used as a -Exxx return value.
> 
> I'm not sure this is the proper fix, but at least we get rid of the
> dead code which pretended to check for access faults.
> 
> Signed-off-by: Rasmus Villemoes 
> ---

Acked-by: Shannon Nelson 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] intel: i40e: fix confused code

[Nelson, Shannon]

> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> Sent: Saturday, October 17, 2015 1:58 PM
> Subject: [PATCH] intel: i40e: fix confused code
> 
> This code is pretty confused. The variable name 'bytes_not_copied'
> clearly indicates that the programmer knew the semantics of
> copy_{to,from}_user, but then the return value is checked for being
> negative and used as a -Exxx return value.
> 
> I'm not sure this is the proper fix, but at least we get rid of the
> dead code which pretended to check for access faults.
> 
> Signed-off-by: Rasmus Villemoes 

I believe this patch is unnecessary: if the value is negative, then it already 
is an error code giving some potentially useful information.  When I dig into 
the copy_to_user() code, I see in the comments for put_user() that -EFAULT is 
the error being returned.  Also, if somewhere else along the line there is some 
other error, I'd prefer to return that value rather than stomp on it with my 
own error code.  

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] intel: i40e: fix confused code

[Nelson, Shannon]

> From: Rasmus Villemoes [mailto:li...@rasmusvillemoes.dk]
> Sent: Saturday, October 17, 2015 1:58 PM
> Subject: [PATCH] intel: i40e: fix confused code
> 
> This code is pretty confused. The variable name 'bytes_not_copied'
> clearly indicates that the programmer knew the semantics of
> copy_{to,from}_user, but then the return value is checked for being
> negative and used as a -Exxx return value.
> 
> I'm not sure this is the proper fix, but at least we get rid of the
> dead code which pretended to check for access faults.
> 
> Signed-off-by: Rasmus Villemoes 

I believe this patch is unnecessary: if the value is negative, then it already 
is an error code giving some potentially useful information.  When I dig into 
the copy_to_user() code, I see in the comments for put_user() that -EFAULT is 
the error being returned.  Also, if somewhere else along the line there is some 
other error, I'd prefer to return that value rather than stomp on it with my 
own error code.  

sln

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [E1000-devel] [PATCH 0/4] i40e: Neatening and object size reductions

[Nelson, Shannon]

> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Tuesday, September 03, 2013 6:31 PM
> 
> On Wed, 2013-09-04 at 01:00 +, Nelson, Shannon wrote:
> 
> Hi Shannon.
> 
> > > -Original Message- > From: Joe Perches
> > [mailto:j...@perches.com] > Sent: Friday, August 30, 2013 4:06 PM > >
> > Just some potential cleanings...
> >
> > >   i40e: Whitespace cleaning
> >
> > Hmmm, we hadn't noticed the new experimental "--fix" option before.
> > There are a lot of good suggestions there, but obviously it needs a
> lot
> > of reading and tweaking before it can be used.  There are cases here
> > where function call parameters are adjusted to line up with the
> opening
> > '(' but that pushes the parameter(s) beyond 80 columns - we're trying
> > to stay within the 80 column line and checkpatch clean.  Also, there
> > are several where the first continued parameter line indent is changed
> > but the next line or two are not.
> >
> > We'll spend time going through these and try to take care of what
> makes
> > sense.
> 
> Swell.  All these are your choice to fix as you want.
> 
> Exceeding 80 columns doesn't bother me much.

We should perhaps become a little more flexible ourselves, but we've finally 
got a good process going internally, including this as a check.  I don't dare 
disturb the machine now that it is working :-).

> Keeping alignment appropriate for multi-line statements
> needs work inside checkpatch.  I played with it a bit
> but it's unfortunately complicated by intermixed
> insertions and deletions.

Yeah, it all gets a little funky after a while.

> 
> > >   i40e: Add and use pf_
> >
> > We had considered this kind of macro awhile ago, but nixed it for a
> few
> > different reasons, but primarily because it seems like
> > yet-another-print-macro and not necessarily worth the effort.
> >
> > >   i40e: pf_ remove "%s: " ... __func__
> >
> > We're beginning to remove many of the __func__ uses, so these prints
> > are no longer all doing the __func__ thing.  We originally had them
> > there for early development and debugging and are currently removing
> > them from the normal path messages.
> 
> Fine by me.  I think __func__ is nearly always pretty
> useless myself.

It was useful for a while, but it is time to be pulling it out.

> 
> > >   i40e: Convert pf_ macros to functions
> >
> > Doesn't this create a problem with polluting the kernel namespace?
> > These don't apply to any other driver.  I suppose we could lessen the
> > namespace problem with i40e_ prefix, but I'm still not sold on it.  I
> > suspect we can still get much of the text savings replacing the
> > __func__ with __builtin_return_address(0) where needed, and remove
> them
> > where no longer needed.  Does that work for you?
> 
> I think you could just as soon whatever combinations of the
> other standard logging mechanisms without using pf_
> 
>   wiphy_
>   netif_
>   netdev_
>   dev_
>   pr_
> 
> as appropriate.  I did that only because there was ~10K
> of what I think of as not too useful function names out
> of a defconfig size of 140k.

Yes, and I think removing much of the __func__ or using 
__builtin_return_address(0) will help.

> 
> > >   i40e: Fix 32 bit shift compilation warnings
> >
> > Sure.
> 
> I think you should use the kernel.h standard macros
> for lower_32_bits and upper_32_bits instead.

Yep.

> 
> cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [E1000-devel] [PATCH 0/4] i40e: Neatening and object size reductions

[Nelson, Shannon]

> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Friday, August 30, 2013 4:06 PM
> 
> Just some potential cleanings...

>   i40e: Whitespace cleaning

Hmmm, we hadn't noticed the new experimental "--fix" option before.  There are 
a lot of good suggestions there, but obviously it needs a lot of reading and 
tweaking before it can be used.  There are cases here where function call 
parameters are adjusted to line up with the opening '(' but that pushes the 
parameter(s) beyond 80 columns - we're trying to stay within the 80 column line 
and checkpatch clean.  Also, there are several where the first continued 
parameter line indent is changed but the next line or two are not.

We'll spend time going through these and try to take care of what makes sense.

>   i40e: Add and use pf_

We had considered this kind of macro awhile ago, but nixed it for a few 
different reasons, but primarily because it seems like yet-another-print-macro 
and not necessarily worth the effort.

>   i40e: pf_ remove "%s: " ... __func__

We're beginning to remove many of the __func__ uses, so these prints are no 
longer all doing the __func__ thing.  We originally had them there for early 
development and debugging and are currently removing them from the normal path 
messages.

>   i40e: Convert pf_ macros to functions

Doesn't this create a problem with polluting the kernel namespace?  These don't 
apply to any other driver.  I suppose we could lessen the namespace problem 
with i40e_ prefix, but I'm still not sold on it.  I suspect we can still get 
much of the text savings replacing the __func__ with 
__builtin_return_address(0) where needed, and remove them where no longer 
needed.  Does that work for you? 

>   i40e: Fix 32 bit shift compilation warnings

Sure.

sln



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [E1000-devel] [PATCH 0/4] i40e: Neatening and object size reductions

[Nelson, Shannon]

 -Original Message-
 From: Joe Perches [mailto:j...@perches.com]
 Sent: Friday, August 30, 2013 4:06 PM
 
 Just some potential cleanings...

   i40e: Whitespace cleaning

Hmmm, we hadn't noticed the new experimental --fix option before.  There are 
a lot of good suggestions there, but obviously it needs a lot of reading and 
tweaking before it can be used.  There are cases here where function call 
parameters are adjusted to line up with the opening '(' but that pushes the 
parameter(s) beyond 80 columns - we're trying to stay within the 80 column line 
and checkpatch clean.  Also, there are several where the first continued 
parameter line indent is changed but the next line or two are not.

We'll spend time going through these and try to take care of what makes sense.

   i40e: Add and use pf_level

We had considered this kind of macro awhile ago, but nixed it for a few 
different reasons, but primarily because it seems like yet-another-print-macro 
and not necessarily worth the effort.

   i40e: pf_level remove %s:  ... __func__

We're beginning to remove many of the __func__ uses, so these prints are no 
longer all doing the __func__ thing.  We originally had them there for early 
development and debugging and are currently removing them from the normal path 
messages.

   i40e: Convert pf_level macros to functions

Doesn't this create a problem with polluting the kernel namespace?  These don't 
apply to any other driver.  I suppose we could lessen the namespace problem 
with i40e_ prefix, but I'm still not sold on it.  I suspect we can still get 
much of the text savings replacing the __func__ with 
__builtin_return_address(0) where needed, and remove them where no longer 
needed.  Does that work for you? 

   i40e: Fix 32 bit shift compilation warnings

Sure.

sln



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [E1000-devel] [PATCH 0/4] i40e: Neatening and object size reductions

[Nelson, Shannon]

 -Original Message-
 From: Joe Perches [mailto:j...@perches.com]
 Sent: Tuesday, September 03, 2013 6:31 PM
 
 On Wed, 2013-09-04 at 01:00 +, Nelson, Shannon wrote:
 
 Hi Shannon.
 
   -Original Message-  From: Joe Perches
  [mailto:j...@perches.com]  Sent: Friday, August 30, 2013 4:06 PM  
  Just some potential cleanings...
 
 i40e: Whitespace cleaning
 
  Hmmm, we hadn't noticed the new experimental --fix option before.
  There are a lot of good suggestions there, but obviously it needs a
 lot
  of reading and tweaking before it can be used.  There are cases here
  where function call parameters are adjusted to line up with the
 opening
  '(' but that pushes the parameter(s) beyond 80 columns - we're trying
  to stay within the 80 column line and checkpatch clean.  Also, there
  are several where the first continued parameter line indent is changed
  but the next line or two are not.
 
  We'll spend time going through these and try to take care of what
 makes
  sense.
 
 Swell.  All these are your choice to fix as you want.
 
 Exceeding 80 columns doesn't bother me much.

We should perhaps become a little more flexible ourselves, but we've finally 
got a good process going internally, including this as a check.  I don't dare 
disturb the machine now that it is working :-).

 Keeping alignment appropriate for multi-line statements
 needs work inside checkpatch.  I played with it a bit
 but it's unfortunately complicated by intermixed
 insertions and deletions.

Yeah, it all gets a little funky after a while.

 
 i40e: Add and use pf_level
 
  We had considered this kind of macro awhile ago, but nixed it for a
 few
  different reasons, but primarily because it seems like
  yet-another-print-macro and not necessarily worth the effort.
 
 i40e: pf_level remove %s:  ... __func__
 
  We're beginning to remove many of the __func__ uses, so these prints
  are no longer all doing the __func__ thing.  We originally had them
  there for early development and debugging and are currently removing
  them from the normal path messages.
 
 Fine by me.  I think __func__ is nearly always pretty
 useless myself.

It was useful for a while, but it is time to be pulling it out.

 
 i40e: Convert pf_level macros to functions
 
  Doesn't this create a problem with polluting the kernel namespace?
  These don't apply to any other driver.  I suppose we could lessen the
  namespace problem with i40e_ prefix, but I'm still not sold on it.  I
  suspect we can still get much of the text savings replacing the
  __func__ with __builtin_return_address(0) where needed, and remove
 them
  where no longer needed.  Does that work for you?
 
 I think you could just as soon whatever combinations of the
 other standard logging mechanisms without using pf_level
 
   wiphy_level
   netif_level
   netdev_level
   dev_level
   pr_level
 
 as appropriate.  I did that only because there was ~10K
 of what I think of as not too useful function names out
 of a defconfig size of 140k.

Yes, and I think removing much of the __func__ or using 
__builtin_return_address(0) will help.

 
 i40e: Fix 32 bit shift compilation warnings
 
  Sure.
 
 I think you should use the kernel.h standard macros
 for lower_32_bits and upper_32_bits instead.

Yep.

 
 cheers, Joe

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: ioatdma Self-test copy timeout

[Nelson, Shannon]

>-Original Message-
>From: Tomas Hlavacek [mailto:[EMAIL PROTECTED] 
>Sent: Sunday, February 17, 2008 6:22 PM
>To: Nelson, Shannon; linux-kernel@vger.kernel.org
>Subject: ioatdma Self-test copy timeout
>
>Short description: In 2.6.25-rc2 ioatdma driver fails to 
>initialize due 
>to Self-test timeout when the ioatdma is linked into the kernel.
>
>
>Long descr: When I select to compile ioatdma into the kernel I get:
>
>ACPI: PCI Interrupt :00:08.0[A] -> GSI 16 (level, low) -> IRQ 16
>PCI: Setting latency timer of device :00:08.0 to 64
>ioatdma :00:08.0: Intel(R) I/OAT DMA Engine found, 4 channels, 
>device version 0x12, driver version 2.04
>ioatdma: ioat_dma_test_callback(8086)
>ioatdma :00:08.0: Self-test copy timed out, disabling
>ioatdma :00:08.0: Intel(R) I/OAT DMA Engine initialization failed
>ACPI: PCI interrupt for device :00:08.0 disabled
>
>As a kernel module ioatdma initializes just fine with the same 
>kernel on 
>the same HW without any other changes than selecting it to be 
>a module. 
>And it also succeeded to initialize when i tried to do this:
>
>--- a/drivers/dma/ioat_dma.c2008-02-17 01:52:19.0 +0100
>+++ b/drivers/dma/ioat_dma.c2008-02-18 02:47:27.0 +0100
>@@ -1089,7 +1089,7 @@
>goto free_resources;
>}
>device->common.device_issue_pending(dma_chan);
>-   msleep(1);
>+   msleep(100);
> 
>if (device->common.device_is_tx_complete(dma_chan, 
>cookie, NULL, 
>NULL)
>!= DMA_SUCCESS) {

Hmmm - interesting.  I'm not good enough with the kernel startup
sequence to know what it might be waiting for.  I don't see any problem
with a longer wait other than perhaps delaying the kernel startup.  We
left it a module and never really worked with it linked into the kernel,
primarily because its performance can be dependant on the work load, and
some folks will want the flexibility that a module gives.  We didn't let
it autoload at boot time for a similar reason.

If you want it to autoload at boot, you might want to add a modprobe
command to an init script.

sln
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RFC v3 4/7] dmaengine: Add slave DMA interface

[Nelson, Shannon]

>From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
>Sent: Monday, February 18, 2008 5:30 AM
>To: Nelson, Shannon
>Cc: Haavard Skinnemoen; Williams, Dan J; 
>linux-kernel@vger.kernel.org; David Brownell; 
>[EMAIL PROTECTED]; Francis Moreau; Paul Mundt; Vladimir A. 
>Barinov; Pierre Ossman
>Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
>
>On Fri, 15 Feb 2008 09:12:35 -0800
>"Nelson, Shannon" <[EMAIL PROTECTED]> wrote:
>
>> I'll jump in here briefly - I'm okay with the direction this 
>is going,
>> but I want to be protective of ioatdma performance.  As used 
>in struct
>> ioat_desc_sw, the cookie and ack elements end up very close 
>to the end
>> of a cache line and I'd like them to not get pushed out across the
>> boundry.  I don't think this proposal changes the layout, I'm just
>> bringing up my concern.
>
>Sure, performance is very important, and it's good to see that you're
>critical about the changes I'm proposing. That said, the memory layout
>doesn't change at all with this patch -- the fields that didn't go into
>the generic dma descriptor were at the end of the struct to begin with.
>
>I can add a comment saying that cookie and ack must always come first.
>Any other fields that we need to be careful about?
>
>Haavard
>

Those are the only two that I'm worried about at the moment.  I'm just
hoping that a quirk in some compiler's struct packing doesn't push them
over that edge.

Thanks,
sln
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RFC v3 4/7] dmaengine: Add slave DMA interface

[Nelson, Shannon]

From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
Sent: Monday, February 18, 2008 5:30 AM
To: Nelson, Shannon
Cc: Haavard Skinnemoen; Williams, Dan J; 
linux-kernel@vger.kernel.org; David Brownell; 
[EMAIL PROTECTED]; Francis Moreau; Paul Mundt; Vladimir A. 
Barinov; Pierre Ossman
Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface

On Fri, 15 Feb 2008 09:12:35 -0800
Nelson, Shannon [EMAIL PROTECTED] wrote:

 I'll jump in here briefly - I'm okay with the direction this 
is going,
 but I want to be protective of ioatdma performance.  As used 
in struct
 ioat_desc_sw, the cookie and ack elements end up very close 
to the end
 of a cache line and I'd like them to not get pushed out across the
 boundry.  I don't think this proposal changes the layout, I'm just
 bringing up my concern.

Sure, performance is very important, and it's good to see that you're
critical about the changes I'm proposing. That said, the memory layout
doesn't change at all with this patch -- the fields that didn't go into
the generic dma descriptor were at the end of the struct to begin with.

I can add a comment saying that cookie and ack must always come first.
Any other fields that we need to be careful about?

Haavard


Those are the only two that I'm worried about at the moment.  I'm just
hoping that a quirk in some compiler's struct packing doesn't push them
over that edge.

Thanks,
sln
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: ioatdma Self-test copy timeout

[Nelson, Shannon]

-Original Message-
From: Tomas Hlavacek [mailto:[EMAIL PROTECTED] 
Sent: Sunday, February 17, 2008 6:22 PM
To: Nelson, Shannon; linux-kernel@vger.kernel.org
Subject: ioatdma Self-test copy timeout

Short description: In 2.6.25-rc2 ioatdma driver fails to 
initialize due 
to Self-test timeout when the ioatdma is linked into the kernel.


Long descr: When I select to compile ioatdma into the kernel I get:

ACPI: PCI Interrupt :00:08.0[A] - GSI 16 (level, low) - IRQ 16
PCI: Setting latency timer of device :00:08.0 to 64
ioatdma :00:08.0: Intel(R) I/OAT DMA Engine found, 4 channels, 
device version 0x12, driver version 2.04
ioatdma: ioat_dma_test_callback(8086)
ioatdma :00:08.0: Self-test copy timed out, disabling
ioatdma :00:08.0: Intel(R) I/OAT DMA Engine initialization failed
ACPI: PCI interrupt for device :00:08.0 disabled

As a kernel module ioatdma initializes just fine with the same 
kernel on 
the same HW without any other changes than selecting it to be 
a module. 
And it also succeeded to initialize when i tried to do this:

--- a/drivers/dma/ioat_dma.c2008-02-17 01:52:19.0 +0100
+++ b/drivers/dma/ioat_dma.c2008-02-18 02:47:27.0 +0100
@@ -1089,7 +1089,7 @@
goto free_resources;
}
device-common.device_issue_pending(dma_chan);
-   msleep(1);
+   msleep(100);
 
if (device-common.device_is_tx_complete(dma_chan, 
cookie, NULL, 
NULL)
!= DMA_SUCCESS) {

Hmmm - interesting.  I'm not good enough with the kernel startup
sequence to know what it might be waiting for.  I don't see any problem
with a longer wait other than perhaps delaying the kernel startup.  We
left it a module and never really worked with it linked into the kernel,
primarily because its performance can be dependant on the work load, and
some folks will want the flexibility that a module gives.  We didn't let
it autoload at boot time for a similar reason.

If you want it to autoload at boot, you might want to add a modprobe
command to an init script.

sln
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RFC v3 4/7] dmaengine: Add slave DMA interface

[Nelson, Shannon]

>From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
>Sent: Friday, February 15, 2008 1:53 AM
>To: Haavard Skinnemoen
>Cc: Williams, Dan J; linux-kernel@vger.kernel.org; Nelson, 
>Shannon; David Brownell; [EMAIL PROTECTED]; Francis 
>Moreau; Paul Mundt; Vladimir A. Barinov; Pierre Ossman
>Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface
>
>On Wed, 13 Feb 2008 20:24:02 +0100
>Haavard Skinnemoen <[EMAIL PROTECTED]> wrote:
>
>> But looking at your latest patch series, I guess we can use the new
>> "next" field instead. It's not like we really need the full
>> capabilities of list_head.
>
>On second thought, if we do this, we would be using the "next" field in
>an undocumented way. It is currently documented as follows:
>
> * @next: at completion submit this descriptor
>
>But that's not how we're going to use it when doing slave transfers:
>We're using it to keep track of all the descriptors that have already
>been submitted.
>
>I think it would actually be better to go the other way and have the
>async_tx API extend the descriptor as well for its private fields. That
>way, we get the pointer we need, but we can document it differently.
>
>So how about we do something along the lines of the patch below? We
>need to update all the users too of course, but apart from making the
>dma_slave_descriptor struct smaller, I don't think it will change the
>actual memory layout at all.
>
>Haavard
>
>diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>index 9bb3407..abaf324 100644
>--- a/include/linux/dmaengine.h
>+++ b/include/linux/dmaengine.h
>@@ -267,8 +267,7 @@ struct dma_client {
> 
> typedef void (*dma_async_tx_callback)(void *dma_async_param);
> /**
>- * struct dma_async_tx_descriptor - async transaction descriptor
>- * ---dma generic offload fields---
>+ * struct dma_descriptor - generic dma offload descriptor
>  * @cookie: tracking cookie for this transaction, set to -EBUSY if
>  *this tx is sitting on a dependency list
>  * @ack: the descriptor can not be reused until the client 
>acknowledges
>@@ -280,12 +279,8 @@ typedef void 
>(*dma_async_tx_callback)(void *dma_async_param);
>  * @tx_submit: set the prepared descriptor(s) to be executed 
>by the engine
>  * @callback: routine to call after this operation is complete
>  * @callback_param: general parameter to pass to the callback routine
>- * ---async_tx api specific fields---
>- * @next: at completion submit this descriptor
>- * @parent: pointer to the next level up in the dependency chain
>- * @lock: protect the parent and next pointers
>  */
>-struct dma_async_tx_descriptor {
>+struct dma_descriptor {
>   dma_cookie_t cookie;
>   int ack;
>   dma_addr_t phys;
>@@ -294,6 +289,17 @@ struct dma_async_tx_descriptor {
>   dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx);
>   dma_async_tx_callback callback;
>   void *callback_param;
>+};
>+
>+/**
>+ * struct dma_async_tx_descriptor - async transaction descriptor
>+ * @dma: generic dma descriptor
>+ * @next: at completion submit this descriptor
>+ * @parent: pointer to the next level up in the dependency chain
>+ * @lock: protect the parent and next pointers
>+ */
>+struct dma_async_tx_descriptor {
>+  struct dma_descriptor dma;
>   struct dma_async_tx_descriptor *next;
>   struct dma_async_tx_descriptor *parent;
>   spinlock_t lock;
>@@ -301,13 +307,13 @@ struct dma_async_tx_descriptor {
> 
> /**
>  * struct dma_slave_descriptor - extended DMA descriptor for slave DMA
>- * @async_tx: async transaction descriptor
>- * @client_node: for use by the client, for example when operating on
>- *scatterlists.
>+ * @dma: generic dma descriptor
>+ * @next: for use by the client, for example to keep track of
>+ *submitted descriptors when dealing with scatterlists.
>  */
> struct dma_slave_descriptor {
>-  struct dma_async_tx_descriptor txd;
>-  struct list_head client_node;
>+  struct dma_descriptor dma;
>+  struct dma_slave_descriptor *next;
> };
> 
> /**
>

I'll jump in here briefly - I'm okay with the direction this is going,
but I want to be protective of ioatdma performance.  As used in struct
ioat_desc_sw, the cookie and ack elements end up very close to the end
of a cache line and I'd like them to not get pushed out across the
boundry.  I don't think this proposal changes the layout, I'm just
bringing up my concern.

Selfishly,
sln
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RFC v3 4/7] dmaengine: Add slave DMA interface

[Nelson, Shannon]

From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
Sent: Friday, February 15, 2008 1:53 AM
To: Haavard Skinnemoen
Cc: Williams, Dan J; linux-kernel@vger.kernel.org; Nelson, 
Shannon; David Brownell; [EMAIL PROTECTED]; Francis 
Moreau; Paul Mundt; Vladimir A. Barinov; Pierre Ossman
Subject: Re: [RFC v3 4/7] dmaengine: Add slave DMA interface

On Wed, 13 Feb 2008 20:24:02 +0100
Haavard Skinnemoen [EMAIL PROTECTED] wrote:

 But looking at your latest patch series, I guess we can use the new
 next field instead. It's not like we really need the full
 capabilities of list_head.

On second thought, if we do this, we would be using the next field in
an undocumented way. It is currently documented as follows:

 * @next: at completion submit this descriptor

But that's not how we're going to use it when doing slave transfers:
We're using it to keep track of all the descriptors that have already
been submitted.

I think it would actually be better to go the other way and have the
async_tx API extend the descriptor as well for its private fields. That
way, we get the pointer we need, but we can document it differently.

So how about we do something along the lines of the patch below? We
need to update all the users too of course, but apart from making the
dma_slave_descriptor struct smaller, I don't think it will change the
actual memory layout at all.

Haavard

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 9bb3407..abaf324 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -267,8 +267,7 @@ struct dma_client {
 
 typedef void (*dma_async_tx_callback)(void *dma_async_param);
 /**
- * struct dma_async_tx_descriptor - async transaction descriptor
- * ---dma generic offload fields---
+ * struct dma_descriptor - generic dma offload descriptor
  * @cookie: tracking cookie for this transaction, set to -EBUSY if
  *this tx is sitting on a dependency list
  * @ack: the descriptor can not be reused until the client 
acknowledges
@@ -280,12 +279,8 @@ typedef void 
(*dma_async_tx_callback)(void *dma_async_param);
  * @tx_submit: set the prepared descriptor(s) to be executed 
by the engine
  * @callback: routine to call after this operation is complete
  * @callback_param: general parameter to pass to the callback routine
- * ---async_tx api specific fields---
- * @next: at completion submit this descriptor
- * @parent: pointer to the next level up in the dependency chain
- * @lock: protect the parent and next pointers
  */
-struct dma_async_tx_descriptor {
+struct dma_descriptor {
   dma_cookie_t cookie;
   int ack;
   dma_addr_t phys;
@@ -294,6 +289,17 @@ struct dma_async_tx_descriptor {
   dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx);
   dma_async_tx_callback callback;
   void *callback_param;
+};
+
+/**
+ * struct dma_async_tx_descriptor - async transaction descriptor
+ * @dma: generic dma descriptor
+ * @next: at completion submit this descriptor
+ * @parent: pointer to the next level up in the dependency chain
+ * @lock: protect the parent and next pointers
+ */
+struct dma_async_tx_descriptor {
+  struct dma_descriptor dma;
   struct dma_async_tx_descriptor *next;
   struct dma_async_tx_descriptor *parent;
   spinlock_t lock;
@@ -301,13 +307,13 @@ struct dma_async_tx_descriptor {
 
 /**
  * struct dma_slave_descriptor - extended DMA descriptor for slave DMA
- * @async_tx: async transaction descriptor
- * @client_node: for use by the client, for example when operating on
- *scatterlists.
+ * @dma: generic dma descriptor
+ * @next: for use by the client, for example to keep track of
+ *submitted descriptors when dealing with scatterlists.
  */
 struct dma_slave_descriptor {
-  struct dma_async_tx_descriptor txd;
-  struct list_head client_node;
+  struct dma_descriptor dma;
+  struct dma_slave_descriptor *next;
 };
 
 /**


I'll jump in here briefly - I'm okay with the direction this is going,
but I want to be protective of ioatdma performance.  As used in struct
ioat_desc_sw, the cookie and ack elements end up very close to the end
of a cache line and I'd like them to not get pushed out across the
boundry.  I don't think this proposal changes the layout, I'm just
bringing up my concern.

Selfishly,
sln
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH][I/OAT]: Remove duplicate assignation in dma_skb_copy_datagram_iovec

[Nelson, Shannon]

>-Original Message-
>From: Brice Goglin [mailto:[EMAIL PROTECTED] 
>Sent: Wednesday, February 13, 2008 1:05 PM
>To: Nelson, Shannon; Williams, Dan J
>Cc: Leech, Christopher; LKML
>Subject: [PATCH][I/OAT]: Remove duplicate assignation in 
>dma_skb_copy_datagram_iovec
>
>[I/OAT]: Remove duplicate assignation in dma_skb_copy_datagram_iovec
>
>No need to compute copy twice in the frags loop in
>dma_skb_copy_datagram_iovec().
>
>Signed-off-by: Brice Goglin <[EMAIL PROTECTED]>
>---
> user_dma.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/core/user_dma.c b/net/core/user_dma.c
>index 0ad1cd5..c77aff9 100644
>--- a/net/core/user_dma.c
>+++ b/net/core/user_dma.c
>@@ -75,7 +75,7 @@ int dma_skb_copy_datagram_iovec(struct 
>dma_chan *chan,
> 
>   end = start + skb_shinfo(skb)->frags[i].size;
>   copy = end - offset;
>-  if ((copy = end - offset) > 0) {
>+  if (copy > 0) {
>   skb_frag_t *frag = _shinfo(skb)->frags[i];
>   struct page *page = frag->page;
> 

Acked-by: Shannon Nelson <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/4] async_tx: fix multiple dependency submission

[Nelson, Shannon]

>-Original Message-
>From: Williams, Dan J 
>Sent: Tuesday, February 12, 2008 11:03 PM
>To: linux-kernel@vger.kernel.org
>Cc: [EMAIL PROTECTED]; Nelson, Shannon; [EMAIL PROTECTED]; 
>[EMAIL PROTECTED]
>Subject: [PATCH 2/4] async_tx: fix multiple dependency submission
>
>Shrink struct dma_async_tx_descriptor and introduce
>async_tx_channel_switch to properly inject a channel switch 
>interrupt in
>the descriptor stream.  This simplifies the locking model as drivers no
>longer need to handle dma_async_tx_descriptor.lock.
>
>Signed-off-by: Dan Williams <[EMAIL PROTECTED]>
>---
>
> crypto/async_tx/async_tx.c |  197 
>
> drivers/dma/dmaengine.c|2 
> drivers/dma/iop-adma.c |9 +-
> include/linux/dmaengine.h  |9 +-
> 4 files changed, 170 insertions(+), 47 deletions(-)
>
>


Acked-by: Shannon Nelson <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 3/4] async_tx: kill ->device_dependency_added

[Nelson, Shannon]

>-Original Message-
>From: Williams, Dan J 
>Sent: Tuesday, February 12, 2008 11:03 PM
>To: linux-kernel@vger.kernel.org
>Cc: [EMAIL PROTECTED]; Nelson, Shannon; [EMAIL PROTECTED]; 
>[EMAIL PROTECTED]
>Subject: [PATCH 3/4] async_tx: kill ->device_dependency_added
>
>DMA drivers no longer need to be notified of depdency 
>submission events as
>async_tx_run_dependencies and async_tx_channel_switch will handle the
>scheduling and execution of dependent operations.
>
>Signed-off-by: Dan Williams <[EMAIL PROTECTED]>
>---
>
> drivers/dma/dmaengine.c   |1 -
> drivers/dma/ioat_dma.c|   12 
> drivers/dma/iop-adma.c|7 ---
> include/linux/dmaengine.h |2 --
> 4 files changed, 0 insertions(+), 22 deletions(-)
>
>
>diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>index 9369781..5ca0d94 100644
>--- a/drivers/dma/dmaengine.c
>+++ b/drivers/dma/dmaengine.c
>@@ -362,7 +362,6 @@ int dma_async_device_register(struct 
>dma_device *device)
> 
>   BUG_ON(!device->device_alloc_chan_resources);
>   BUG_ON(!device->device_free_chan_resources);
>-  BUG_ON(!device->device_dependency_added);
>   BUG_ON(!device->device_is_tx_complete);
>   BUG_ON(!device->device_issue_pending);
>   BUG_ON(!device->dev);
>diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
>index dff38ac..05ace54 100644
>--- a/drivers/dma/ioat_dma.c
>+++ b/drivers/dma/ioat_dma.c
>@@ -922,17 +922,6 @@ static void 
>ioat_dma_memcpy_cleanup(struct ioat_dma_chan *ioat_chan)
>   spin_unlock_bh(_chan->cleanup_lock);
> }
> 
>-static void ioat_dma_dependency_added(struct dma_chan *chan)
>-{
>-  struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
>-  spin_lock_bh(_chan->desc_lock);
>-  if (ioat_chan->pending == 0) {
>-  spin_unlock_bh(_chan->desc_lock);
>-  ioat_dma_memcpy_cleanup(ioat_chan);
>-  } else
>-  spin_unlock_bh(_chan->desc_lock);
>-}
>-
> /**
>  * ioat_dma_is_complete - poll the status of a IOAT DMA transaction
>  * @chan: IOAT DMA channel handle
>@@ -1314,7 +1303,6 @@ struct ioatdma_device 
>*ioat_dma_probe(struct pci_dev *pdev,
> 
>   dma_cap_set(DMA_MEMCPY, device->common.cap_mask);
>   device->common.device_is_tx_complete = ioat_dma_is_complete;
>-  device->common.device_dependency_added = 
>ioat_dma_dependency_added;
>   switch (device->version) {
>   case IOAT_VER_1_2:
>   device->common.device_prep_dma_memcpy = 
>ioat1_dma_prep_memcpy;
>diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
>index a6171da..1cb4284 100644
>--- a/drivers/dma/iop-adma.c
>+++ b/drivers/dma/iop-adma.c
>@@ -672,12 +672,6 @@ iop_adma_prep_dma_zero_sum(struct 
>dma_chan *chan, dma_addr_t *dma_src,
>   return sw_desc ? _desc->async_tx : NULL;
> }
> 
>-static void iop_adma_dependency_added(struct dma_chan *chan)
>-{
>-  struct iop_adma_chan *iop_chan = to_iop_adma_chan(chan);
>-  tasklet_schedule(_chan->irq_tasklet);
>-}
>-
> static void iop_adma_free_chan_resources(struct dma_chan *chan)
> {
>   struct iop_adma_chan *iop_chan = to_iop_adma_chan(chan);
>@@ -1178,7 +1172,6 @@ static int __devinit 
>iop_adma_probe(struct platform_device *pdev)
>   dma_dev->device_free_chan_resources = 
>iop_adma_free_chan_resources;
>   dma_dev->device_is_tx_complete = iop_adma_is_complete;
>   dma_dev->device_issue_pending = iop_adma_issue_pending;
>-  dma_dev->device_dependency_added = iop_adma_dependency_added;
>   dma_dev->dev = >dev;
> 
>   /* set prep routines based on capability */
>diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>index d04b169..e2538b4 100644
>--- a/include/linux/dmaengine.h
>+++ b/include/linux/dmaengine.h
>@@ -258,7 +258,6 @@ struct dma_async_tx_descriptor {
>  * @device_prep_dma_zero_sum: prepares a zero_sum operation
>  * @device_prep_dma_memset: prepares a memset operation
>  * @device_prep_dma_interrupt: prepares an end of chain 
>interrupt operation
>- * @device_dependency_added: async_tx notifies the channel 
>about new deps
>  * @device_issue_pending: push pending transactions to hardware
>  */
> struct dma_device {
>@@ -293,7 +292,6 @@ struct dma_device {
>   struct dma_async_tx_descriptor *(*device_prep_dma_interrupt)(
>   struct dma_chan *chan);
> 
>-  void (*device_dependency_added)(struct dma_chan *chan);
>   enum dma_status (*device_is_tx_complete)(struct dma_chan *chan,
>   dma_cookie_t cookie, dma_cookie_t *last,
>   dma_cookie_t *used);
>
>

Acked-by: Shannon Nelson <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/4] async_tx: fix multiple dependency submission

[Nelson, Shannon]

-Original Message-
From: Williams, Dan J 
Sent: Tuesday, February 12, 2008 11:03 PM
To: linux-kernel@vger.kernel.org
Cc: [EMAIL PROTECTED]; Nelson, Shannon; [EMAIL PROTECTED]; 
[EMAIL PROTECTED]
Subject: [PATCH 2/4] async_tx: fix multiple dependency submission

Shrink struct dma_async_tx_descriptor and introduce
async_tx_channel_switch to properly inject a channel switch 
interrupt in
the descriptor stream.  This simplifies the locking model as drivers no
longer need to handle dma_async_tx_descriptor.lock.

Signed-off-by: Dan Williams [EMAIL PROTECTED]
---

 crypto/async_tx/async_tx.c |  197 

 drivers/dma/dmaengine.c|2 
 drivers/dma/iop-adma.c |9 +-
 include/linux/dmaengine.h  |9 +-
 4 files changed, 170 insertions(+), 47 deletions(-)




Acked-by: Shannon Nelson [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 3/4] async_tx: kill -device_dependency_added

[Nelson, Shannon]

-Original Message-
From: Williams, Dan J 
Sent: Tuesday, February 12, 2008 11:03 PM
To: linux-kernel@vger.kernel.org
Cc: [EMAIL PROTECTED]; Nelson, Shannon; [EMAIL PROTECTED]; 
[EMAIL PROTECTED]
Subject: [PATCH 3/4] async_tx: kill -device_dependency_added

DMA drivers no longer need to be notified of depdency 
submission events as
async_tx_run_dependencies and async_tx_channel_switch will handle the
scheduling and execution of dependent operations.

Signed-off-by: Dan Williams [EMAIL PROTECTED]
---

 drivers/dma/dmaengine.c   |1 -
 drivers/dma/ioat_dma.c|   12 
 drivers/dma/iop-adma.c|7 ---
 include/linux/dmaengine.h |2 --
 4 files changed, 0 insertions(+), 22 deletions(-)


diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 9369781..5ca0d94 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -362,7 +362,6 @@ int dma_async_device_register(struct 
dma_device *device)
 
   BUG_ON(!device-device_alloc_chan_resources);
   BUG_ON(!device-device_free_chan_resources);
-  BUG_ON(!device-device_dependency_added);
   BUG_ON(!device-device_is_tx_complete);
   BUG_ON(!device-device_issue_pending);
   BUG_ON(!device-dev);
diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
index dff38ac..05ace54 100644
--- a/drivers/dma/ioat_dma.c
+++ b/drivers/dma/ioat_dma.c
@@ -922,17 +922,6 @@ static void 
ioat_dma_memcpy_cleanup(struct ioat_dma_chan *ioat_chan)
   spin_unlock_bh(ioat_chan-cleanup_lock);
 }
 
-static void ioat_dma_dependency_added(struct dma_chan *chan)
-{
-  struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
-  spin_lock_bh(ioat_chan-desc_lock);
-  if (ioat_chan-pending == 0) {
-  spin_unlock_bh(ioat_chan-desc_lock);
-  ioat_dma_memcpy_cleanup(ioat_chan);
-  } else
-  spin_unlock_bh(ioat_chan-desc_lock);
-}
-
 /**
  * ioat_dma_is_complete - poll the status of a IOAT DMA transaction
  * @chan: IOAT DMA channel handle
@@ -1314,7 +1303,6 @@ struct ioatdma_device 
*ioat_dma_probe(struct pci_dev *pdev,
 
   dma_cap_set(DMA_MEMCPY, device-common.cap_mask);
   device-common.device_is_tx_complete = ioat_dma_is_complete;
-  device-common.device_dependency_added = 
ioat_dma_dependency_added;
   switch (device-version) {
   case IOAT_VER_1_2:
   device-common.device_prep_dma_memcpy = 
ioat1_dma_prep_memcpy;
diff --git a/drivers/dma/iop-adma.c b/drivers/dma/iop-adma.c
index a6171da..1cb4284 100644
--- a/drivers/dma/iop-adma.c
+++ b/drivers/dma/iop-adma.c
@@ -672,12 +672,6 @@ iop_adma_prep_dma_zero_sum(struct 
dma_chan *chan, dma_addr_t *dma_src,
   return sw_desc ? sw_desc-async_tx : NULL;
 }
 
-static void iop_adma_dependency_added(struct dma_chan *chan)
-{
-  struct iop_adma_chan *iop_chan = to_iop_adma_chan(chan);
-  tasklet_schedule(iop_chan-irq_tasklet);
-}
-
 static void iop_adma_free_chan_resources(struct dma_chan *chan)
 {
   struct iop_adma_chan *iop_chan = to_iop_adma_chan(chan);
@@ -1178,7 +1172,6 @@ static int __devinit 
iop_adma_probe(struct platform_device *pdev)
   dma_dev-device_free_chan_resources = 
iop_adma_free_chan_resources;
   dma_dev-device_is_tx_complete = iop_adma_is_complete;
   dma_dev-device_issue_pending = iop_adma_issue_pending;
-  dma_dev-device_dependency_added = iop_adma_dependency_added;
   dma_dev-dev = pdev-dev;
 
   /* set prep routines based on capability */
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index d04b169..e2538b4 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -258,7 +258,6 @@ struct dma_async_tx_descriptor {
  * @device_prep_dma_zero_sum: prepares a zero_sum operation
  * @device_prep_dma_memset: prepares a memset operation
  * @device_prep_dma_interrupt: prepares an end of chain 
interrupt operation
- * @device_dependency_added: async_tx notifies the channel 
about new deps
  * @device_issue_pending: push pending transactions to hardware
  */
 struct dma_device {
@@ -293,7 +292,6 @@ struct dma_device {
   struct dma_async_tx_descriptor *(*device_prep_dma_interrupt)(
   struct dma_chan *chan);
 
-  void (*device_dependency_added)(struct dma_chan *chan);
   enum dma_status (*device_is_tx_complete)(struct dma_chan *chan,
   dma_cookie_t cookie, dma_cookie_t *last,
   dma_cookie_t *used);



Acked-by: Shannon Nelson [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH][I/OAT]: Remove duplicate assignation in dma_skb_copy_datagram_iovec

[Nelson, Shannon]

-Original Message-
From: Brice Goglin [mailto:[EMAIL PROTECTED] 
Sent: Wednesday, February 13, 2008 1:05 PM
To: Nelson, Shannon; Williams, Dan J
Cc: Leech, Christopher; LKML
Subject: [PATCH][I/OAT]: Remove duplicate assignation in 
dma_skb_copy_datagram_iovec

[I/OAT]: Remove duplicate assignation in dma_skb_copy_datagram_iovec

No need to compute copy twice in the frags loop in
dma_skb_copy_datagram_iovec().

Signed-off-by: Brice Goglin [EMAIL PROTECTED]
---
 user_dma.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/user_dma.c b/net/core/user_dma.c
index 0ad1cd5..c77aff9 100644
--- a/net/core/user_dma.c
+++ b/net/core/user_dma.c
@@ -75,7 +75,7 @@ int dma_skb_copy_datagram_iovec(struct 
dma_chan *chan,
 
   end = start + skb_shinfo(skb)-frags[i].size;
   copy = end - offset;
-  if ((copy = end - offset)  0) {
+  if (copy  0) {
   skb_frag_t *frag = skb_shinfo(skb)-frags[i];
   struct page *page = frag-page;
 

Acked-by: Shannon Nelson [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: IOATDMA: driver oopses on request_irq()

[Nelson, Shannon]

>-Original Message-
>From: Chuck Ebbert [mailto:[EMAIL PROTECTED] 
>Sent: Thursday, January 17, 2008 4:01 PM
>To: Nelson, Shannon
>Cc: linux-kernel
>Subject: IOATDMA: driver oopses on request_irq()
>
>https://bugzilla.redhat.com/show_bug.cgi?id=311991
>[still present in 2.6.23.8]
>
>[Drivers really need to actually be ready to receive interrupts when
>they call request_irq(). We keep finding drivers that aren't because
>we enable CONFIG_DEBUG_SHIRQ]
>
>kernel version: 2.6.22.7-85.fc7debug
>modprobe ioatdma during bootup sez:
>
>[  142.497353] Module ioatdma cannot be unloaded due to unsafe usage in
>drivers/dma/ioatdma.c:829
>[  142.497906] ACPI: PCI Interrupt :00:08.0[A] -> GSI 16 
>(level, low) -> IRQ 16
>[  142.497945] Unable to handle kernel NULL pointer dereference at
>0003 RIP: 
>[  142.498002]  [] 
>:ioatdma:ioat_do_interrupt+0xd/0x50
>[  142.498770] PGD 10fc46067 PUD 114a56067 PMD 0 
>[  142.499156] Oops:  [1] SMP 
>[  142.499498] last sysfs file: /block/sr0/removable
>[  142.499753] CPU 3 
>[  142.500049] Modules linked in: ioatdma dell_rbu nfsd 
>exportfs lockd nfs_acl
>ipmi_devintf ipmi_si ipmi_msghandler hidp l2cap bluetooth sunr
>pc xt_length ipt_TOS xt_state xt_tcpudp ipt_REJECT ipt_LOG xt_limit
>iptable_mangle iptable_nat nf_nat nf_conntrack_ipv4 
>nf_conntrack nfnetlin
>k iptable_filter ip_tables x_tables ppp_deflate zlib_deflate 
>ppp_synctty
>ppp_async crc_ccitt ppp_generic slhc bridge dm_mirror dm_multipath d
>m_mod video sbs button dock battery ac tcp_westwood kvm_intel 
>kvm sr_mod cdrom
>ata_generic ata_piix serio_raw libata bnx2 rtc_cmos sg joydev 
>usb_storage shpchp megaraid_sas sd_mod scsi_mod ext3 jbd 
>mbcache ehci_hcd
>ohci_hcd uhci_hcd
>[  142.505129] Pid: 5239, comm: modprobe Not tainted 
>2.6.22.7-85.fc7debug #1
>[  142.505386] RIP: 0010:[]  []
>:ioatdma:ioat_do_interrupt+0xd/0x50
>[  142.505892] RSP: 0018:81010ff83c48  EFLAGS: 00010096
>[  142.506146] RAX: 1214 RBX: 0202 
>RCX: 8835b0f5
>[  142.506405] RDX:  RSI: 81012cc11700 
>RDI: 08f4
>[  142.506663] RBP: 81012cc11700 R08: 81012cc11700 
>R09: 81012cc11700
>[  142.506919] R10: 005a R11: 811f2016 
>R12: 0020
>[  142.507176] R13: 81012cc11700 R14: 8835adb3 
>R15: 08f4
>[  142.507434] FS:  2aae0250() GS:810105855900()
>knlGS:
>[  142.507892] CS:  0010 DS:  ES:  CR0: 8005003b
>[  142.508147] CR2: 0003 CR3: 000110281000 
>CR4: 26e0
>[  142.508406] Process modprobe (pid: 5239, threadinfo 
>81010ff82000, task
>8101106dcf60)
>[  142.508866] Stack:  81012cc11700 0202 
>81012447ab40
>81069bd8
>[  142.509544]  81012447ac80 8835b0f5 
>01ff81012f971070 81012f971070
>[  142.510176]  81012cc11700 81012f971000 
>fff4 
>[  142.510566] Call Trace:
>[  142.511063]  [] request_irq+0xcc/0x120
>[  142.511321]  [] :ioatdma:ioat_probe+0x15b/0x457
>[  142.511581]  [] _spin_unlock+0x17/0x20
>[  142.511839]  [] pci_device_probe+0xcd/0x134
>[  142.512097]  [] driver_probe_device+0xff/0x17c
>[  142.512354]  [] __driver_attach+0x90/0xcc
>[  142.512609]  [] __driver_attach+0x0/0xcc
>[  142.512865]  [] __driver_attach+0x0/0xcc
>[  142.513120]  [] bus_for_each_dev+0x43/0x6e
>[  142.513378]  [] bus_add_driver+0x7b/0x19d
>[  142.513634]  [] __pci_register_driver+0x68/0x9a
>[  142.513892]  [] sys_init_module+0x163f/0x17a1
>[  142.514153]  [] audit_syscall_entry+0x141/0x174
>[  142.514414]  [] tracesys+0xd5/0xda
>[  142.514673] 
>[  142.514920] 
>[  142.514920] Code: 8a 42 03 0f b6 d8 31 c0 f6 c3 01 74 31 f6 
>c3 02 75 0a 0f b6 
>[  142.516541] RIP  [] 
>:ioatdma:ioat_do_interrupt+0xd/0x50
>[  142.516841]  RSP 
>[  142.517091] CR2: 0003


Chuck - We did some significant rework on the ioatdma driver last
summer.  Unfortunately it didn't make it into 2.6.23, but it is
available in 2.6.24 as well as out-of-core on our SourceForge site.
Please try the newer code - I believe it will fix your problem.

Thanks,
sln
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: IOATDMA: driver oopses on request_irq()

[Nelson, Shannon]

-Original Message-
From: Chuck Ebbert [mailto:[EMAIL PROTECTED] 
Sent: Thursday, January 17, 2008 4:01 PM
To: Nelson, Shannon
Cc: linux-kernel
Subject: IOATDMA: driver oopses on request_irq()

https://bugzilla.redhat.com/show_bug.cgi?id=311991
[still present in 2.6.23.8]

[Drivers really need to actually be ready to receive interrupts when
they call request_irq(). We keep finding drivers that aren't because
we enable CONFIG_DEBUG_SHIRQ]

kernel version: 2.6.22.7-85.fc7debug
modprobe ioatdma during bootup sez:

[  142.497353] Module ioatdma cannot be unloaded due to unsafe usage in
drivers/dma/ioatdma.c:829
[  142.497906] ACPI: PCI Interrupt :00:08.0[A] - GSI 16 
(level, low) - IRQ 16
[  142.497945] Unable to handle kernel NULL pointer dereference at
0003 RIP: 
[  142.498002]  [8835adc0] 
:ioatdma:ioat_do_interrupt+0xd/0x50
[  142.498770] PGD 10fc46067 PUD 114a56067 PMD 0 
[  142.499156] Oops:  [1] SMP 
[  142.499498] last sysfs file: /block/sr0/removable
[  142.499753] CPU 3 
[  142.500049] Modules linked in: ioatdma dell_rbu nfsd 
exportfs lockd nfs_acl
ipmi_devintf ipmi_si ipmi_msghandler hidp l2cap bluetooth sunr
pc xt_length ipt_TOS xt_state xt_tcpudp ipt_REJECT ipt_LOG xt_limit
iptable_mangle iptable_nat nf_nat nf_conntrack_ipv4 
nf_conntrack nfnetlin
k iptable_filter ip_tables x_tables ppp_deflate zlib_deflate 
ppp_synctty
ppp_async crc_ccitt ppp_generic slhc bridge dm_mirror dm_multipath d
m_mod video sbs button dock battery ac tcp_westwood kvm_intel 
kvm sr_mod cdrom
ata_generic ata_piix serio_raw libata bnx2 rtc_cmos sg joydev 
usb_storage shpchp megaraid_sas sd_mod scsi_mod ext3 jbd 
mbcache ehci_hcd
ohci_hcd uhci_hcd
[  142.505129] Pid: 5239, comm: modprobe Not tainted 
2.6.22.7-85.fc7debug #1
[  142.505386] RIP: 0010:[8835adc0]  [8835adc0]
:ioatdma:ioat_do_interrupt+0xd/0x50
[  142.505892] RSP: 0018:81010ff83c48  EFLAGS: 00010096
[  142.506146] RAX: 1214 RBX: 0202 
RCX: 8835b0f5
[  142.506405] RDX:  RSI: 81012cc11700 
RDI: 08f4
[  142.506663] RBP: 81012cc11700 R08: 81012cc11700 
R09: 81012cc11700
[  142.506919] R10: 005a R11: 811f2016 
R12: 0020
[  142.507176] R13: 81012cc11700 R14: 8835adb3 
R15: 08f4
[  142.507434] FS:  2aae0250() GS:810105855900()
knlGS:
[  142.507892] CS:  0010 DS:  ES:  CR0: 8005003b
[  142.508147] CR2: 0003 CR3: 000110281000 
CR4: 26e0
[  142.508406] Process modprobe (pid: 5239, threadinfo 
81010ff82000, task
8101106dcf60)
[  142.508866] Stack:  81012cc11700 0202 
81012447ab40
81069bd8
[  142.509544]  81012447ac80 8835b0f5 
01ff81012f971070 81012f971070
[  142.510176]  81012cc11700 81012f971000 
fff4 
[  142.510566] Call Trace:
[  142.511063]  [81069bd8] request_irq+0xcc/0x120
[  142.511321]  [8835aab7] :ioatdma:ioat_probe+0x15b/0x457
[  142.511581]  [8126a916] _spin_unlock+0x17/0x20
[  142.511839]  [8112c1be] pci_device_probe+0xcd/0x134
[  142.512097]  [811a54a3] driver_probe_device+0xff/0x17c
[  142.512354]  [811a5668] __driver_attach+0x90/0xcc
[  142.512609]  [811a55d8] __driver_attach+0x0/0xcc
[  142.512865]  [811a55d8] __driver_attach+0x0/0xcc
[  142.513120]  [811a4824] bus_for_each_dev+0x43/0x6e
[  142.513378]  [811a4b9c] bus_add_driver+0x7b/0x19d
[  142.513634]  [8112c3a0] __pci_register_driver+0x68/0x9a
[  142.513892]  [81055f70] sys_init_module+0x163f/0x17a1
[  142.514153]  [81067540] audit_syscall_entry+0x141/0x174
[  142.514414]  [81009d2e] tracesys+0xd5/0xda
[  142.514673] 
[  142.514920] 
[  142.514920] Code: 8a 42 03 0f b6 d8 31 c0 f6 c3 01 74 31 f6 
c3 02 75 0a 0f b6 
[  142.516541] RIP  [8835adc0] 
:ioatdma:ioat_do_interrupt+0xd/0x50
[  142.516841]  RSP 81010ff83c48
[  142.517091] CR2: 0003


Chuck - We did some significant rework on the ioatdma driver last
summer.  Unfortunately it didn't make it into 2.6.23, but it is
available in 2.6.24 as well as out-of-core on our SourceForge site.
Please try the newer code - I believe it will fix your problem.

Thanks,
sln
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/4] async_tx: kill tx_set_src and tx_set_dest methods

[Nelson, Shannon]

>From: Williams, Dan J 
>
>The tx_set_src and tx_set_dest methods were originally 
>implemented to allow
>an array of addresses to be passed down from async_xor to the dmaengine
>driver while minimizing stack overhead.  Removing these methods allows
>drivers to have all transaction parameters available at 'prep' 
>time, saves
>two function pointers in struct dma_async_tx_descriptor, and 
>reduces the
>number of indirect branches..
>
>A consequence of moving this data to the 'prep' routine is that
>multi-source routines like async_xor need temporary storage to 
>convert an
>array of linear addresses into an array of dma addresses.  In 
>order to keep
>the same stack footprint of the previous implementation the 
>input array is
>reused as storage for the dma addresses.  This requires that
>sizeof(dma_addr_t) be less than or equal to sizeof(void *).  As a
>consequence CONFIG_DMADEVICES now depends on 
>!CONFIG_HIGHMEM64G.  It also
>requires that drivers be able to make descriptor resources 
>available when
>the 'prep' routine is polled.
>
>Signed-off-by: Dan Williams <[EMAIL PROTECTED]>
>---
[... Snip ...]

Sorry I took so long to get back to this.  The ioatdma related changes
look fine - I like this approach much better.

Acked by: Shannon Nelson <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 3/4] async_tx: replace 'int_en' with operation preparationflags

[Nelson, Shannon]

>From: Williams, Dan J 
>
>Pass a full set of flags to drivers' per-operation 'prep' routines.
>Currently the only flag passed is DMA_PREP_INTERRUPT.  The 
>expectation is
>that arch-specific async_tx_find_channel() implementations can 
>exploit this
>capability to find the best channel for an operation.
>
>Signed-off-by: Dan Williams <[EMAIL PROTECTED]>
>---
>
> crypto/async_tx/async_memcpy.c |3 ++-
> crypto/async_tx/async_memset.c |3 ++-
> crypto/async_tx/async_xor.c|   10 ++
> drivers/dma/ioat_dma.c |4 ++--
> drivers/dma/iop-adma.c |   20 ++--
> include/asm-arm/arch-iop13xx/adma.h|   18 ++
> include/asm-arm/hardware/iop3xx-adma.h |   30 
>+-
> include/linux/dmaengine.h  |   17 +
> 8 files changed, 62 insertions(+), 43 deletions(-)


Yep - this is good.  This will allow us to add an interrupt request flag
to the transaction request in the future.

Acked by: Shannon Nelson <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/4] async_tx: kill tx_set_src and tx_set_dest methods

[Nelson, Shannon]

From: Williams, Dan J 

The tx_set_src and tx_set_dest methods were originally 
implemented to allow
an array of addresses to be passed down from async_xor to the dmaengine
driver while minimizing stack overhead.  Removing these methods allows
drivers to have all transaction parameters available at 'prep' 
time, saves
two function pointers in struct dma_async_tx_descriptor, and 
reduces the
number of indirect branches..

A consequence of moving this data to the 'prep' routine is that
multi-source routines like async_xor need temporary storage to 
convert an
array of linear addresses into an array of dma addresses.  In 
order to keep
the same stack footprint of the previous implementation the 
input array is
reused as storage for the dma addresses.  This requires that
sizeof(dma_addr_t) be less than or equal to sizeof(void *).  As a
consequence CONFIG_DMADEVICES now depends on 
!CONFIG_HIGHMEM64G.  It also
requires that drivers be able to make descriptor resources 
available when
the 'prep' routine is polled.

Signed-off-by: Dan Williams [EMAIL PROTECTED]
---
[... Snip ...]

Sorry I took so long to get back to this.  The ioatdma related changes
look fine - I like this approach much better.

Acked by: Shannon Nelson [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 3/4] async_tx: replace 'int_en' with operation preparationflags

[Nelson, Shannon]

From: Williams, Dan J 

Pass a full set of flags to drivers' per-operation 'prep' routines.
Currently the only flag passed is DMA_PREP_INTERRUPT.  The 
expectation is
that arch-specific async_tx_find_channel() implementations can 
exploit this
capability to find the best channel for an operation.

Signed-off-by: Dan Williams [EMAIL PROTECTED]
---

 crypto/async_tx/async_memcpy.c |3 ++-
 crypto/async_tx/async_memset.c |3 ++-
 crypto/async_tx/async_xor.c|   10 ++
 drivers/dma/ioat_dma.c |4 ++--
 drivers/dma/iop-adma.c |   20 ++--
 include/asm-arm/arch-iop13xx/adma.h|   18 ++
 include/asm-arm/hardware/iop3xx-adma.h |   30 
+-
 include/linux/dmaengine.h  |   17 +
 8 files changed, 62 insertions(+), 43 deletions(-)


Yep - this is good.  This will allow us to add an interrupt request flag
to the transaction request in the future.

Acked by: Shannon Nelson [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [2.6 patch] #if 0 dma_async_memcpy_buf_to_buf()

[Nelson, Shannon]

>From: Alan Cox [mailto:[EMAIL PROTECTED] 
>Sent: Tuesday, January 01, 2008 6:26 AM
>To: Adrian Bunk
>Cc: Nelson, Shannon; Williams, Dan J; linux-kernel@vger.kernel.org
>Subject: Re: [2.6 patch] #if 0 dma_async_memcpy_buf_to_buf()
>
>On Tue, 1 Jan 2008 15:49:24 +0200
>Adrian Bunk <[EMAIL PROTECTED]> wrote:
>
>> This patch #if 0's the unused dma_async_memcpy_buf_to_buf().
>> 
>> Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]>
>
>NAK. Yet again you are trying to remove bits of APIs in ways that make
>no sense.
>

Yes, this should be a NAK.

Thank you Alan.

sln
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [2.6 patch] #if 0 dma_async_memcpy_buf_to_buf()

[Nelson, Shannon]

From: Alan Cox [mailto:[EMAIL PROTECTED] 
Sent: Tuesday, January 01, 2008 6:26 AM
To: Adrian Bunk
Cc: Nelson, Shannon; Williams, Dan J; linux-kernel@vger.kernel.org
Subject: Re: [2.6 patch] #if 0 dma_async_memcpy_buf_to_buf()

On Tue, 1 Jan 2008 15:49:24 +0200
Adrian Bunk [EMAIL PROTECTED] wrote:

 This patch #if 0's the unused dma_async_memcpy_buf_to_buf().
 
 Signed-off-by: Adrian Bunk [EMAIL PROTECTED]

NAK. Yet again you are trying to remove bits of APIs in ways that make
no sense.


Yes, this should be a NAK.

Thank you Alan.

sln
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] drivers/dma/ioat_dma.c: inlining failed

[Nelson, Shannon]

>-Original Message-
>From: S.Çağlar Onur [mailto:[EMAIL PROTECTED] 
>Sent: Friday, December 14, 2007 11:45 AM
>To: linux-kernel@vger.kernel.org
>Cc: Nelson, Shannon
>Subject: [PATCH] drivers/dma/ioat_dma.c: inlining failed
>
>Hi;
>
>After commit 7bb67c14fd3778504fb77da30ce11582336dfced, Linus's 
>git tree gaves following compiliation error with gcc-3.4.6. 
>Following patch solves this issue for me;
>
>
>[...]
>  CC [M]  drivers/dma/ioat.o
>  CC [M]  drivers/dma/ioat_dma.o
>drivers/dma/ioat_dma.c: In function `ioat1_tx_submit':
>drivers/dma/ioat_dma.c:177: sorry, unimplemented: inlining 
>failed in call to '__ioat1_dma_memcpy_issue_pending': function 
>body not available
>drivers/dma/ioat_dma.c:268: sorry, unimplemented: called from here
>make[2]: *** [drivers/dma/ioat_dma.o] Hata 1
>make[1]: *** [drivers/dma] Hata 2
>make: *** [drivers] Hata 2
>
>Signed-off-by: S.Çağlar Onur <[EMAIL PROTECTED]>
[...]

Yep.  I posted a similar patch with a couple more tweaks after a month ago 
after Andrew's comments, which hasn't yet moved from -mm to Linus' tree.  See 
http://lkml.org/lkml/2007/11/16/336

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2] dmaengine: Simple DMA memcpy test client

[Nelson, Shannon]

>From: Olof Johansson [mailto:[EMAIL PROTECTED] 
>Sent: Thursday, December 13, 2007 7:32 PM
>To: Haavard Skinnemoen
>Cc: Nelson, Shannon; Williams, Dan J; 
>linux-kernel@vger.kernel.org; Sam Ravnborg
>Subject: Re: [PATCH v2] dmaengine: Simple DMA memcpy test client
>
>On Fri, Nov 23, 2007 at 04:34:36PM +0100, Haavard Skinnemoen wrote:
>> This client tests DMA memcpy using various lengths and 
>various offsets
>> into the source and destination buffers. It will initialize both
>> buffers with a know pattern and verify that the DMA engine copies the
>> requested region and nothing more.
>> 
>> Signed-off-by: Haavard Skinnemoen <[EMAIL PROTECTED]>
>
>Hi,
>
>First of all: Thanks for sharing this, it's quite useful! I've been
>playing around with this a bit today, and I've been seeing some odd
>behaviour.
>
>It seems that once a channel is allocated to dmatest, it will never be
>freed, i.e. the drivers device_free_chan_resources will never be called
>on it. Looking at the dma stack, I'm not sure just where it's expected
>to happen. Once the channel is allocated, the dma_chan_get/put 
>calls all
>just modify the per-cpu variables, and nothing will ever boil down to a
>call to kref_put() of the refcount until the _driver_ is deregistered.
>Not even deregistering the client seems to do it.
>
>Or am I missing something here? Shannon? Dan?
>
>I happened to catch it due to a BUG_ON() in my 
>device_alloc_chan_resources
>that checked to make sure it wasn't allocated twice without a free
>inbetween. It hit on the second load of the dmatest module, since they
>were never freed on unload.
>
>
>-Olof
>

No, you're not missing anything, you've read it right.  Notice at the
top of the ioatdma's ioat_dma_alloc_chan_resources() we check to see if
we've already been here.

I suspect this is a hangover from the earlier dmaengine design where TCP
was the only client and it never released things, so the only time we
needed to free the resources was when ioatdma was unloaded.

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2] dmaengine: Simple DMA memcpy test client

[Nelson, Shannon]

From: Olof Johansson [mailto:[EMAIL PROTECTED] 
Sent: Thursday, December 13, 2007 7:32 PM
To: Haavard Skinnemoen
Cc: Nelson, Shannon; Williams, Dan J; 
linux-kernel@vger.kernel.org; Sam Ravnborg
Subject: Re: [PATCH v2] dmaengine: Simple DMA memcpy test client

On Fri, Nov 23, 2007 at 04:34:36PM +0100, Haavard Skinnemoen wrote:
 This client tests DMA memcpy using various lengths and 
various offsets
 into the source and destination buffers. It will initialize both
 buffers with a know pattern and verify that the DMA engine copies the
 requested region and nothing more.
 
 Signed-off-by: Haavard Skinnemoen [EMAIL PROTECTED]

Hi,

First of all: Thanks for sharing this, it's quite useful! I've been
playing around with this a bit today, and I've been seeing some odd
behaviour.

It seems that once a channel is allocated to dmatest, it will never be
freed, i.e. the drivers device_free_chan_resources will never be called
on it. Looking at the dma stack, I'm not sure just where it's expected
to happen. Once the channel is allocated, the dma_chan_get/put 
calls all
just modify the per-cpu variables, and nothing will ever boil down to a
call to kref_put() of the refcount until the _driver_ is deregistered.
Not even deregistering the client seems to do it.

Or am I missing something here? Shannon? Dan?

I happened to catch it due to a BUG_ON() in my 
device_alloc_chan_resources
that checked to make sure it wasn't allocated twice without a free
inbetween. It hit on the second load of the dmatest module, since they
were never freed on unload.


-Olof


No, you're not missing anything, you've read it right.  Notice at the
top of the ioatdma's ioat_dma_alloc_chan_resources() we check to see if
we've already been here.

I suspect this is a hangover from the earlier dmaengine design where TCP
was the only client and it never released things, so the only time we
needed to free the resources was when ioatdma was unloaded.

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] drivers/dma/ioat_dma.c: inlining failed

[Nelson, Shannon]

-Original Message-
From: S.Çağlar Onur [mailto:[EMAIL PROTECTED] 
Sent: Friday, December 14, 2007 11:45 AM
To: linux-kernel@vger.kernel.org
Cc: Nelson, Shannon
Subject: [PATCH] drivers/dma/ioat_dma.c: inlining failed

Hi;

After commit 7bb67c14fd3778504fb77da30ce11582336dfced, Linus's 
git tree gaves following compiliation error with gcc-3.4.6. 
Following patch solves this issue for me;


[...]
  CC [M]  drivers/dma/ioat.o
  CC [M]  drivers/dma/ioat_dma.o
drivers/dma/ioat_dma.c: In function `ioat1_tx_submit':
drivers/dma/ioat_dma.c:177: sorry, unimplemented: inlining 
failed in call to '__ioat1_dma_memcpy_issue_pending': function 
body not available
drivers/dma/ioat_dma.c:268: sorry, unimplemented: called from here
make[2]: *** [drivers/dma/ioat_dma.o] Hata 1
make[1]: *** [drivers/dma] Hata 2
make: *** [drivers] Hata 2

Signed-off-by: S.Çağlar Onur [EMAIL PROTECTED]
[...]

Yep.  I posted a similar patch with a couple more tweaks after a month ago 
after Andrew's comments, which hasn't yet moved from -mm to Linus' tree.  See 
http://lkml.org/lkml/2007/11/16/336

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] dmaengine: Simple DMA memcpy test client

[Nelson, Shannon]

>-Original Message-
>From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
>
>This client tests DMA memcpy using various lengths and various offsets
>into the source and destination buffers. It will initialize both
>buffers with a know pattern and verify that the DMA engine copies the
>requested region and nothing more.
>
>Signed-off-by: Haavard Skinnemoen <[EMAIL PROTECTED]>
>---

[...]

>diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
>new file mode 100644
>index 000..d9e9866
>--- /dev/null
>+++ b/drivers/dma/dmatest.c
>@@ -0,0 +1,272 @@
>+/*
>+ * DMA Engine test module
>+ *
>+ * Copyright (C) 2007 Atmel Corporation
>+ *
>+ * This program is free software; you can redistribute it 
>and/or modify
>+ * it under the terms of the GNU General Public License version 2 as
>+ * published by the Free Software Foundation.
>+ */
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+
>+#define TEST_BUF_SIZE (16384)

You might make this a module parameter so we can test with various
sizes.


>+
>+#define SRC_PATTERN   0x7c
>+#define SRC_PATTERN_OUTSIDE   0X8d
>+#define POISON_UNINIT 0x49
>+#define POISON_OUTSIDE0x37
>+
>+struct dmatest {
>+  spinlock_t  lock;
>+  struct dma_client   client;
>+  struct task_struct  *thread;
>+  struct dma_chan *chan;
>+  wait_queue_head_t   wq;
>+  u8  *srcbuf;
>+  u8  *dstbuf;
>+};
>+
>+static inline struct dmatest *to_dmatest(struct dma_client *client)
>+{
>+  return container_of(client, struct dmatest, client);
>+}
>+
>+static enum dma_state_client
>+dmatest_event(struct dma_client *client, struct dma_chan *chan,
>+  enum dma_state state)
>+{
>+  struct dmatest  *test = to_dmatest(client);
>+  enum dma_state_client   ack = DMA_DUP;

DMA_NAK is better default for DMA_RESOURCE_AVAILABLE once you've got the
channel you want, as that will stop DMAEngine from handing you more, and
doesn't bother the DMA_RESOURCE_REMOVED process.

>+
>+  spin_lock(>lock);
>+  switch (state) {
>+  case DMA_RESOURCE_AVAILABLE:
>+  if (!test->chan) {
>+  printk(KERN_INFO "dmatest: Got channel %s\n",
>+  chan->dev.bus_id);
>+  test->chan = chan;
>+  wake_up_interruptible(>wq);
>+  ack = DMA_ACK;
>+  }
>+  break;

Do you ever want to test a specific channel?  Is there a way to identify
specific channels, perhaps by checking the PCI bus/chan/func?  This
could be useful in debugging specific hardware issues - a frilly extra
feature and very HW specific, but potentially useful.  I suppose this
might also depend upon your hardware - is there more than one channel in
your gizmo?

>+
>+  case DMA_RESOURCE_REMOVED:
>+  if (test->chan == chan) {

Should you use your test->lock here? 

>+  printk(KERN_INFO "dmatest: Lost channel %s\n",
>+  chan->dev.bus_id);
>+  test->chan = NULL;
>+  ack = DMA_ACK;
>+  }
>+  break;
>+
>+  default:

Since this is a test program, perhaps a printk() here might be useful...

>+  break;
>+  }
>+  spin_unlock(>lock);
>+
>+  return ack;
>+}
>+
>+static unsigned long dmatest_random(void)
>+{
>+  unsigned long buf;
>+
>+  get_random_bytes(, sizeof(buf));
>+  return buf;
>+}
>+
>+static unsigned int dmatest_verify(u8 *buf, unsigned int start,
>+  unsigned int end, u8 expected)
>+{
>+  unsigned int i;
>+  unsigned int error_count = 0;
>+
>+  for (i = start; i < end; i++) {
>+  if (buf[i] != expected) {
>+  if (error_count < 32)
>+  printk(KERN_ERR "dmatest: 
>buf[0x%x] = %02x "
>+  "(expected %02x)\n",
>+  i, buf[i], expected);
>+  error_count++;
>+  }
>+  }
>+
>+  if (error_count > 32)
>+  printk(KERN_ERR "dmatest: %u errors suppressed\n",
>+  error_count - 32);
>+
>+  return error_count;
>+}
>+
>+static int dmatest_func(void *data)
>+{
>+  struct dmatest  *test = data;
>+  struct dma_chan *chan;
>+  boolshould_stop = false;
>+  unsigned intsrc_off, dst_off, len;
>+  unsigned interror_count;
>+  dma_cookie_tcookie;
>+  enum dma_status status;
>+
>+  dma_cap_set(DMA_MEMCPY, test->client.cap_mask);
>+  dma_async_client_register(>client);
>+  dma_async_client_chan_request(>client);
>+
>+  for (;;) {
>+  DEFINE_WAIT(chan_wait);
>+
>+  pr_debug("dmatest: Waiting for a channel...\n");
>+  for (;;) {
>+  

RE: [PATCH] dmaengine: Simple DMA memcpy test client

[Nelson, Shannon]

-Original Message-
From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 

This client tests DMA memcpy using various lengths and various offsets
into the source and destination buffers. It will initialize both
buffers with a know pattern and verify that the DMA engine copies the
requested region and nothing more.

Signed-off-by: Haavard Skinnemoen [EMAIL PROTECTED]
---

[...]

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
new file mode 100644
index 000..d9e9866
--- /dev/null
+++ b/drivers/dma/dmatest.c
@@ -0,0 +1,272 @@
+/*
+ * DMA Engine test module
+ *
+ * Copyright (C) 2007 Atmel Corporation
+ *
+ * This program is free software; you can redistribute it 
and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include linux/delay.h
+#include linux/dmaengine.h
+#include linux/init.h
+#include linux/kthread.h
+#include linux/module.h
+#include linux/random.h
+#include linux/wait.h
+
+#define TEST_BUF_SIZE (16384)

You might make this a module parameter so we can test with various
sizes.


+
+#define SRC_PATTERN   0x7c
+#define SRC_PATTERN_OUTSIDE   0X8d
+#define POISON_UNINIT 0x49
+#define POISON_OUTSIDE0x37
+
+struct dmatest {
+  spinlock_t  lock;
+  struct dma_client   client;
+  struct task_struct  *thread;
+  struct dma_chan *chan;
+  wait_queue_head_t   wq;
+  u8  *srcbuf;
+  u8  *dstbuf;
+};
+
+static inline struct dmatest *to_dmatest(struct dma_client *client)
+{
+  return container_of(client, struct dmatest, client);
+}
+
+static enum dma_state_client
+dmatest_event(struct dma_client *client, struct dma_chan *chan,
+  enum dma_state state)
+{
+  struct dmatest  *test = to_dmatest(client);
+  enum dma_state_client   ack = DMA_DUP;

DMA_NAK is better default for DMA_RESOURCE_AVAILABLE once you've got the
channel you want, as that will stop DMAEngine from handing you more, and
doesn't bother the DMA_RESOURCE_REMOVED process.

+
+  spin_lock(test-lock);
+  switch (state) {
+  case DMA_RESOURCE_AVAILABLE:
+  if (!test-chan) {
+  printk(KERN_INFO dmatest: Got channel %s\n,
+  chan-dev.bus_id);
+  test-chan = chan;
+  wake_up_interruptible(test-wq);
+  ack = DMA_ACK;
+  }
+  break;

Do you ever want to test a specific channel?  Is there a way to identify
specific channels, perhaps by checking the PCI bus/chan/func?  This
could be useful in debugging specific hardware issues - a frilly extra
feature and very HW specific, but potentially useful.  I suppose this
might also depend upon your hardware - is there more than one channel in
your gizmo?

+
+  case DMA_RESOURCE_REMOVED:
+  if (test-chan == chan) {

Should you use your test-lock here? 

+  printk(KERN_INFO dmatest: Lost channel %s\n,
+  chan-dev.bus_id);
+  test-chan = NULL;
+  ack = DMA_ACK;
+  }
+  break;
+
+  default:

Since this is a test program, perhaps a printk() here might be useful...

+  break;
+  }
+  spin_unlock(test-lock);
+
+  return ack;
+}
+
+static unsigned long dmatest_random(void)
+{
+  unsigned long buf;
+
+  get_random_bytes(buf, sizeof(buf));
+  return buf;
+}
+
+static unsigned int dmatest_verify(u8 *buf, unsigned int start,
+  unsigned int end, u8 expected)
+{
+  unsigned int i;
+  unsigned int error_count = 0;
+
+  for (i = start; i  end; i++) {
+  if (buf[i] != expected) {
+  if (error_count  32)
+  printk(KERN_ERR dmatest: 
buf[0x%x] = %02x 
+  (expected %02x)\n,
+  i, buf[i], expected);
+  error_count++;
+  }
+  }
+
+  if (error_count  32)
+  printk(KERN_ERR dmatest: %u errors suppressed\n,
+  error_count - 32);
+
+  return error_count;
+}
+
+static int dmatest_func(void *data)
+{
+  struct dmatest  *test = data;
+  struct dma_chan *chan;
+  boolshould_stop = false;
+  unsigned intsrc_off, dst_off, len;
+  unsigned interror_count;
+  dma_cookie_tcookie;
+  enum dma_status status;
+
+  dma_cap_set(DMA_MEMCPY, test-client.cap_mask);
+  dma_async_client_register(test-client);
+  dma_async_client_chan_request(test-client);
+
+  for (;;) {
+  DEFINE_WAIT(chan_wait);
+
+  pr_debug(dmatest: Waiting for a channel...\n);
+  for (;;) {
+  spin_lock(test-lock);
+  

Re: [PATCH][RT] 2.6.24-rc2-rt1 drivers/dma/ioat_dma.c compile fix

[Nelson, Shannon]

On Nov 16, 2007 3:57 AM, Sven-Thorsten Dietrich <[EMAIL PROTECTED]>
wrote:
> Compile fix for new code in -rc2.
> 
> I'm not positive about the insertion point...
> 
> Subject: compile error fix (needs review)
> 
> RT changes __list_splice to require prev and next pointers.
> 
> This changes the use in the new code to list_splice_tail,
> but the optimal insertion point needs to be analyzed.
> 
> Signed-off-by: Sven-Thorsten Dietrich <[EMAIL PROTECTED]>
> 
> ---
>  drivers/dma/ioat_dma.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6.23/drivers/dma/ioat_dma.c
> ===
> --- linux-2.6.23.orig/drivers/dma/ioat_dma.c
> +++ linux-2.6.23/drivers/dma/ioat_dma.c
> @@ -244,7 +244,7 @@ static dma_cookie_t ioat_tx_submit(struc
> /* write address into NextDescriptor field of last desc in
chain */
> to_ioat_desc(ioat_chan->used_desc.prev)->hw->next =
>
first->async_tx.phys;
> -   __list_splice(_chain, ioat_chan->used_desc.prev);
> +   list_splice_tail(_chain, ioat_chan->used_desc.prev);
> 

NAK.

These functions do insertions differently.  The 'prev' is pointing to
the last valid descriptor in the queue and you really want to get the
new_chain stuck on after this.  Your list_splice_tail() will insert the
new_chain just before it which will muck up the order of the DMA
requests.

You might have more success with
list_splice_tail(_chain, ioat_chan->used_desc);
where used_desc points to the whole list, rather than using the .prev
pointer to a specific node.

Please copy me on future ioatdma related comments.

Thanks,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH][RT] 2.6.24-rc2-rt1 drivers/dma/ioat_dma.c compile fix

[Nelson, Shannon]

On Nov 16, 2007 3:57 AM, Sven-Thorsten Dietrich [EMAIL PROTECTED]
wrote:
 Compile fix for new code in -rc2.
 
 I'm not positive about the insertion point...
 
 Subject: compile error fix (needs review)
 
 RT changes __list_splice to require prev and next pointers.
 
 This changes the use in the new code to list_splice_tail,
 but the optimal insertion point needs to be analyzed.
 
 Signed-off-by: Sven-Thorsten Dietrich [EMAIL PROTECTED]
 
 ---
  drivers/dma/ioat_dma.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 Index: linux-2.6.23/drivers/dma/ioat_dma.c
 ===
 --- linux-2.6.23.orig/drivers/dma/ioat_dma.c
 +++ linux-2.6.23/drivers/dma/ioat_dma.c
 @@ -244,7 +244,7 @@ static dma_cookie_t ioat_tx_submit(struc
 /* write address into NextDescriptor field of last desc in
chain */
 to_ioat_desc(ioat_chan-used_desc.prev)-hw-next =

first-async_tx.phys;
 -   __list_splice(new_chain, ioat_chan-used_desc.prev);
 +   list_splice_tail(new_chain, ioat_chan-used_desc.prev);
 

NAK.

These functions do insertions differently.  The 'prev' is pointing to
the last valid descriptor in the queue and you really want to get the
new_chain stuck on after this.  Your list_splice_tail() will insert the
new_chain just before it which will muck up the order of the DMA
requests.

You might have more success with
list_splice_tail(new_chain, ioat_chan-used_desc);
where used_desc points to the whole list, rather than using the .prev
pointer to a specific node.

Please copy me on future ioatdma related comments.

Thanks,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Fix broken device refcounting

[Nelson, Shannon]

>From: Williams, Dan J 
>On Sat, 2007-10-27 at 06:49 -0700, Haavard Skinnemoen wrote:
>> On Fri, 26 Oct 2007 09:36:17 -0700
>> Dan Williams <[EMAIL PROTECTED]> wrote:
>> 
>> > @@ -221,7 +220,6 @@ void dma_chan_cleanup(struct kref *kref)
>> >  {
>> >   struct dma_chan *chan = container_of(kref, struct 
>dma_chan, refcount);
>> >   chan->device->device_free_chan_resources(chan);
>> > - kref_put(>device->refcount, dma_async_device_cleanup);
>> >  }
>> >  EXPORT_SYMBOL(dma_chan_cleanup);
>> 
>> While I can't see any problems with the rest of the patch, I 
>think this
>> part is wrong for the same reasons removing the kref_put() from the
>> class device cleanup function is. I don't see any constraint that
>> guarantees that dma_chan_cleanup() will always be called before
>> dma_dev_release(), which means that "chan" may have been freed before
>> this function gets a chance to run. Please correct me if I'm wrong.
>
>Absolutely right, the driver, not dmaengine, frees the memory so there
>must be a per channel reference on the device to hold off the driver's
>remove routine.
>> 
>> Håvard
>
>So how about this...
>
>---snip---
>dmaengine: Fix broken device refcounting
>
>From: Haavard Skinnemoen <[EMAIL PROTECTED]>
>
>When a DMA device is unregistered, its reference count is decremented
>twice for each channel: Once dma_class_dev_release() and once in
>dma_chan_cleanup(). This may result in the DMA device driver's
>remove() function completing before all channels have been cleaned
>up, causing lots of use-after-free fun.
>
>Fix it by incrementing the device's reference count twice for each
>channel during registration.
>
>Signed-off-by: Haavard Skinnemoen <[EMAIL PROTECTED]>
>[EMAIL PROTECTED]: kill unnecessary client refcounting]
>Signed-off-by: Dan Williams <[EMAIL PROTECTED]>
>---
>
> drivers/dma/dmaengine.c |   17 ++---
> 1 files changed, 6 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>index 84257f7..ec7e871 100644
>--- a/drivers/dma/dmaengine.c
>+++ b/drivers/dma/dmaengine.c
>@@ -186,10 +186,9 @@ static void dma_client_chan_alloc(struct 
>dma_client *client)
>   /* we are done once this client rejects
>* an available resource
>*/
>-  if (ack == DMA_ACK) {
>+  if (ack == DMA_ACK)
>   dma_chan_get(chan);
>-  kref_get(>refcount);
>-  } else if (ack == DMA_NAK)
>+  else if (ack == DMA_NAK)
>   return;
>   }
>   }
>@@ -276,11 +275,8 @@ static void 
>dma_clients_notify_removed(struct dma_chan *chan)
>   /* client was holding resources for this channel so
>* free it
>*/
>-  if (ack == DMA_ACK) {
>+  if (ack == DMA_ACK)
>   dma_chan_put(chan);
>-  kref_put(>device->refcount,
>-  dma_async_device_cleanup);
>-  }
>   }
> 
>   mutex_unlock(_list_mutex);
>@@ -320,11 +316,8 @@ void dma_async_client_unregister(struct 
>dma_client *client)
>   ack = client->event_callback(client, chan,
>   DMA_RESOURCE_REMOVED);
> 
>-  if (ack == DMA_ACK) {
>+  if (ack == DMA_ACK)
>   dma_chan_put(chan);
>-  kref_put(>device->refcount,
>-  dma_async_device_cleanup);
>-  }
>   }
> 
>   list_del(>global_node);
>@@ -401,6 +394,8 @@ int dma_async_device_register(struct 
>dma_device *device)
>   goto err_out;
>   }
> 
>+  /* One for the channel, one of the class device */
>+  kref_get(>refcount);
>   kref_get(>refcount);
>   kref_init(>refcount);
>   chan->slow_ref = 0;
>
>

I tested this in my ioatdma setup and no longer get the panic.  I'm good with 
this if you two are happy with it.  

Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Fix broken device refcounting

[Nelson, Shannon]

From: Williams, Dan J 
On Sat, 2007-10-27 at 06:49 -0700, Haavard Skinnemoen wrote:
 On Fri, 26 Oct 2007 09:36:17 -0700
 Dan Williams [EMAIL PROTECTED] wrote:
 
  @@ -221,7 +220,6 @@ void dma_chan_cleanup(struct kref *kref)
   {
struct dma_chan *chan = container_of(kref, struct 
dma_chan, refcount);
chan-device-device_free_chan_resources(chan);
  - kref_put(chan-device-refcount, dma_async_device_cleanup);
   }
   EXPORT_SYMBOL(dma_chan_cleanup);
 
 While I can't see any problems with the rest of the patch, I 
think this
 part is wrong for the same reasons removing the kref_put() from the
 class device cleanup function is. I don't see any constraint that
 guarantees that dma_chan_cleanup() will always be called before
 dma_dev_release(), which means that chan may have been freed before
 this function gets a chance to run. Please correct me if I'm wrong.

Absolutely right, the driver, not dmaengine, frees the memory so there
must be a per channel reference on the device to hold off the driver's
remove routine.
 
 Håvard

So how about this...

---snip---
dmaengine: Fix broken device refcounting

From: Haavard Skinnemoen [EMAIL PROTECTED]

When a DMA device is unregistered, its reference count is decremented
twice for each channel: Once dma_class_dev_release() and once in
dma_chan_cleanup(). This may result in the DMA device driver's
remove() function completing before all channels have been cleaned
up, causing lots of use-after-free fun.

Fix it by incrementing the device's reference count twice for each
channel during registration.

Signed-off-by: Haavard Skinnemoen [EMAIL PROTECTED]
[EMAIL PROTECTED]: kill unnecessary client refcounting]
Signed-off-by: Dan Williams [EMAIL PROTECTED]
---

 drivers/dma/dmaengine.c |   17 ++---
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 84257f7..ec7e871 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -186,10 +186,9 @@ static void dma_client_chan_alloc(struct 
dma_client *client)
   /* we are done once this client rejects
* an available resource
*/
-  if (ack == DMA_ACK) {
+  if (ack == DMA_ACK)
   dma_chan_get(chan);
-  kref_get(device-refcount);
-  } else if (ack == DMA_NAK)
+  else if (ack == DMA_NAK)
   return;
   }
   }
@@ -276,11 +275,8 @@ static void 
dma_clients_notify_removed(struct dma_chan *chan)
   /* client was holding resources for this channel so
* free it
*/
-  if (ack == DMA_ACK) {
+  if (ack == DMA_ACK)
   dma_chan_put(chan);
-  kref_put(chan-device-refcount,
-  dma_async_device_cleanup);
-  }
   }
 
   mutex_unlock(dma_list_mutex);
@@ -320,11 +316,8 @@ void dma_async_client_unregister(struct 
dma_client *client)
   ack = client-event_callback(client, chan,
   DMA_RESOURCE_REMOVED);
 
-  if (ack == DMA_ACK) {
+  if (ack == DMA_ACK)
   dma_chan_put(chan);
-  kref_put(chan-device-refcount,
-  dma_async_device_cleanup);
-  }
   }
 
   list_del(client-global_node);
@@ -401,6 +394,8 @@ int dma_async_device_register(struct 
dma_device *device)
   goto err_out;
   }
 
+  /* One for the channel, one of the class device */
+  kref_get(device-refcount);
   kref_get(device-refcount);
   kref_init(chan-refcount);
   chan-slow_ref = 0;



I tested this in my ioatdma setup and no longer get the panic.  I'm good with 
this if you two are happy with it.  

Signed-off-by: Shannon Nelson [EMAIL PROTECTED]

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Fix broken device refcounting

[Nelson, Shannon]

>-Original Message-
>From: Nelson, Shannon 
>Sent: Friday, October 26, 2007 10:00 AM
>To: 'Haavard Skinnemoen'
>Cc: Williams, Dan J; linux-kernel@vger.kernel.org; 
>[EMAIL PROTECTED]
>Subject: RE: [PATCH] DMA: Fix broken device refcounting
>
>--
>
>When a channel is removed from dmaengine, too many kref_put() calls
>are made and the device removal happens too soon, usually causing
>a panic.
>
>Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
>---
>
> drivers/dma/dmaengine.c |1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>index 8248992..144a1b7 100644
>--- a/drivers/dma/dmaengine.c
>+++ b/drivers/dma/dmaengine.c
>@@ -131,7 +131,6 @@ static void 
>dma_async_device_cleanup(struct kref *kref);
> static void dma_class_dev_release(struct class_device *cd)
> {
>   struct dma_chan *chan = container_of(cd, struct 
>dma_chan, class_dev);
>-  kref_put(>device->refcount, dma_async_device_cleanup);
> }
> 
> static struct class dma_devclass = {

Of course, to avoid compiler complaints, it might be better as something
like:

static void dma_class_dev_release(struct class_device *cd)
{
-   struct dma_chan *chan = container_of(cd, struct dma_chan,
class_dev);
-   kref_put(>device->refcount, dma_async_device_cleanup);
+   return;
}

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Fix broken device refcounting

[Nelson, Shannon]

>From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
>
>When a DMA device is unregistered, its reference count is decremented
>twice for each channel: Once dma_class_dev_release() and once in
>dma_chan_cleanup(). This may result in the DMA device driver's
>remove() function completing before all channels have been cleaned
>up, causing lots of use-after-free fun.
>
>Fix it by incrementing the device's reference count twice for each
>channel during registration.
>
>Signed-off-by: Haavard Skinnemoen <[EMAIL PROTECTED]>
>---
>I'm not sure if this is the correct way to solve it, but it seems to
>work. The remove() function does not hang, which indicates that the
>device's reference count does drop all the way to zero on
>unregistration, which in turn indicates that it did actually drop
>_below_ zero before.
>
> drivers/dma/dmaengine.c |2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>index 8248992..302eded 100644
>--- a/drivers/dma/dmaengine.c
>+++ b/drivers/dma/dmaengine.c
>@@ -397,6 +397,8 @@ int dma_async_device_register(struct 
>dma_device *device)
>   goto err_out;
>   }
> 
>+  /* One for the channel, one of the class device */
>+  kref_get(>refcount);
>   kref_get(>refcount);
>   kref_init(>refcount);
>   chan->slow_ref = 0;
>-- 
>1.5.2.5
>

As Dan said, we've been discussing this offline, and hadn't come to an
agreement yet.  My version of the patch is the opposite of yours -
instead of adding a kref_get(), I remove one of the kref_put() calls.
--

When a channel is removed from dmaengine, too many kref_put() calls
are made and the device removal happens too soon, usually causing
a panic.

Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
---

 drivers/dma/dmaengine.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 8248992..144a1b7 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -131,7 +131,6 @@ static void dma_async_device_cleanup(struct kref
*kref);
 static void dma_class_dev_release(struct class_device *cd)
 {
struct dma_chan *chan = container_of(cd, struct dma_chan,
class_dev);
-   kref_put(>device->refcount, dma_async_device_cleanup);
 }
 
 static struct class dma_devclass = {
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Fix broken device refcounting

[Nelson, Shannon]

From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 

When a DMA device is unregistered, its reference count is decremented
twice for each channel: Once dma_class_dev_release() and once in
dma_chan_cleanup(). This may result in the DMA device driver's
remove() function completing before all channels have been cleaned
up, causing lots of use-after-free fun.

Fix it by incrementing the device's reference count twice for each
channel during registration.

Signed-off-by: Haavard Skinnemoen [EMAIL PROTECTED]
---
I'm not sure if this is the correct way to solve it, but it seems to
work. The remove() function does not hang, which indicates that the
device's reference count does drop all the way to zero on
unregistration, which in turn indicates that it did actually drop
_below_ zero before.

 drivers/dma/dmaengine.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 8248992..302eded 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -397,6 +397,8 @@ int dma_async_device_register(struct 
dma_device *device)
   goto err_out;
   }
 
+  /* One for the channel, one of the class device */
+  kref_get(device-refcount);
   kref_get(device-refcount);
   kref_init(chan-refcount);
   chan-slow_ref = 0;
-- 
1.5.2.5


As Dan said, we've been discussing this offline, and hadn't come to an
agreement yet.  My version of the patch is the opposite of yours -
instead of adding a kref_get(), I remove one of the kref_put() calls.
--

When a channel is removed from dmaengine, too many kref_put() calls
are made and the device removal happens too soon, usually causing
a panic.

Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
---

 drivers/dma/dmaengine.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 8248992..144a1b7 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -131,7 +131,6 @@ static void dma_async_device_cleanup(struct kref
*kref);
 static void dma_class_dev_release(struct class_device *cd)
 {
struct dma_chan *chan = container_of(cd, struct dma_chan,
class_dev);
-   kref_put(chan-device-refcount, dma_async_device_cleanup);
 }
 
 static struct class dma_devclass = {
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Fix broken device refcounting

[Nelson, Shannon]

-Original Message-
From: Nelson, Shannon 
Sent: Friday, October 26, 2007 10:00 AM
To: 'Haavard Skinnemoen'
Cc: Williams, Dan J; linux-kernel@vger.kernel.org; 
[EMAIL PROTECTED]
Subject: RE: [PATCH] DMA: Fix broken device refcounting

--

When a channel is removed from dmaengine, too many kref_put() calls
are made and the device removal happens too soon, usually causing
a panic.

Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
---

 drivers/dma/dmaengine.c |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 8248992..144a1b7 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -131,7 +131,6 @@ static void 
dma_async_device_cleanup(struct kref *kref);
 static void dma_class_dev_release(struct class_device *cd)
 {
   struct dma_chan *chan = container_of(cd, struct 
dma_chan, class_dev);
-  kref_put(chan-device-refcount, dma_async_device_cleanup);
 }
 
 static struct class dma_devclass = {

Of course, to avoid compiler complaints, it might be better as something
like:

static void dma_class_dev_release(struct class_device *cd)
{
-   struct dma_chan *chan = container_of(cd, struct dma_chan,
class_dev);
-   kref_put(chan-device-refcount, dma_async_device_cleanup);
+   return;
}

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] Remove bogus default y for DMAR and NET_DMA

[Nelson, Shannon]

>From: Jeff Garzik [mailto:[EMAIL PROTECTED] 
>
>Andi Kleen wrote:
>> Index: linux-2.6.24-rc1-hack/arch/x86_64/Kconfig
>> ===
>> --- linux-2.6.24-rc1-hack.orig/arch/x86_64/Kconfig
>> +++ linux-2.6.24-rc1-hack/arch/x86_64/Kconfig
>> @@ -753,7 +753,6 @@ config PCI_DOMAINS
>>  config DMAR
>>  bool "Support for DMA Remapping Devices (EXPERIMENTAL)"
>>  depends on PCI_MSI && ACPI && EXPERIMENTAL
>> -default y
>>  help
>>DMA remapping (DMAR) devices support enables 
>independent address
>>translations for Direct Memory Access (DMA) from devices.
>
>ACK
>
>
>> Index: linux-2.6.24-rc1-hack/drivers/dma/Kconfig
>> ===
>> --- linux-2.6.24-rc1-hack.orig/drivers/dma/Kconfig
>> +++ linux-2.6.24-rc1-hack/drivers/dma/Kconfig
>> @@ -43,7 +43,6 @@ comment "DMA Clients"
>>  config NET_DMA
>>  bool "Network: TCP receive copy offload"
>>  depends on DMA_ENGINE && NET
>> -default y
>>  help
>>This enables the use of DMA engines in the network stack to
>>offload receive copy-to-user operations, freeing CPU cycles.
>
>ACK -- but its arguable that given the current code, its worth setting 
>this if DMA_ENGINE is also set.
>

The intent is to give us a DMA offload for NIC-to-user operations.  At
the moment the ioatdma device is the only gizmo supporting this through
NET_DMA, and is the only DMA_ENGINE supported on x86.  As more devices
are made available through DMA_ENGINE on platforms that don't have
ioatdma, removing the "default y" makes sense.  I agree that this is
ACK-able, but under the "it should just work" philosophy, and with the
Intel servers coming out with the hardware support, it may want to wait
until there are more DMA_ENGINE choices.

In any case, I'll ACK this and let the maintainers decide on the timing.

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Nelson, Shannon]

>From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
>Subject: [PATCH] DMA: Correct invalid assumptions in the Kconfig text
>
>This patch corrects what I hope are invalid assumptions about the DMA
>engine layer: Not only Intel(R) hardware can do DMA, and DMA can be
>used for other things than memcpy and RAID offloading.
>
>At the same time, make the DMA Engine menu visible again on AVR32. I'm
>currently working on a driver for a DMA controller that can do
>mem-to-mem transfers (which is supported by the framework) as well as
>device-to-mem and mem-to-device transfers (not currently supported.)
>
>Signed-off-by: Haavard Skinnemoen <[EMAIL PROTECTED]>
>---
>Don't get me wrong; I think Intel deserves lots of respect for
>creating this framework. But this is also why I got a bit disappointed
>when I discovered that it seems to be less generic than I initially
>hoped.
>
>DMA controllers, which may support plain memcpy acceleration in
>addition to more traditional "slave DMA", are very common in SoC
>devices, and I think Linux needs a common framework for it. The
>existing DMA Engine framework seems to come pretty close already, but
>I think it needs more input from the embedded crowd before it can be
>completely usable on a large number of embedded systems.
>
>I'm not going to suggest any changes to the actual framework for
>2.6.24, but I think the _intention_ of the framework needs to be
>clarified.
>
>Haavard
>
> drivers/dma/Kconfig |   10 ++
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>index 9c91b0f..62a9fe5 100644
>--- a/drivers/dma/Kconfig
>+++ b/drivers/dma/Kconfig
>@@ -3,11 +3,13 @@
> #
> 
> menuconfig DMADEVICES
>-  bool "DMA Offload Engine support"
>-  depends on (PCI && X86) || ARCH_IOP32X || ARCH_IOP33X 
>|| ARCH_IOP13XX
>+  bool "DMA Engine support"
>+  depends on (PCI && X86) || ARCH_IOP32X || ARCH_IOP33X 
>|| ARCH_IOP13XX || AVR32
>   help
>-Intel(R) offload engines enable offloading memory 
>copies in the
>-network stack and RAID operations in the MD driver.
>+DMA engines can do asynchronous data transfers without
>+involving the host CPU. This can be used to offload memory
>+copies in the network stack and RAID operations in the MD
>+driver.
> 
> if DMADEVICES
> 
>-- 
>1.5.3.4
>

Acked-by: Shannon Nelson <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Nelson, Shannon]

From: Haavard Skinnemoen [mailto:[EMAIL PROTECTED] 
Subject: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

This patch corrects what I hope are invalid assumptions about the DMA
engine layer: Not only Intel(R) hardware can do DMA, and DMA can be
used for other things than memcpy and RAID offloading.

At the same time, make the DMA Engine menu visible again on AVR32. I'm
currently working on a driver for a DMA controller that can do
mem-to-mem transfers (which is supported by the framework) as well as
device-to-mem and mem-to-device transfers (not currently supported.)

Signed-off-by: Haavard Skinnemoen [EMAIL PROTECTED]
---
Don't get me wrong; I think Intel deserves lots of respect for
creating this framework. But this is also why I got a bit disappointed
when I discovered that it seems to be less generic than I initially
hoped.

DMA controllers, which may support plain memcpy acceleration in
addition to more traditional slave DMA, are very common in SoC
devices, and I think Linux needs a common framework for it. The
existing DMA Engine framework seems to come pretty close already, but
I think it needs more input from the embedded crowd before it can be
completely usable on a large number of embedded systems.

I'm not going to suggest any changes to the actual framework for
2.6.24, but I think the _intention_ of the framework needs to be
clarified.

Haavard

 drivers/dma/Kconfig |   10 ++
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 9c91b0f..62a9fe5 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -3,11 +3,13 @@
 #
 
 menuconfig DMADEVICES
-  bool DMA Offload Engine support
-  depends on (PCI  X86) || ARCH_IOP32X || ARCH_IOP33X 
|| ARCH_IOP13XX
+  bool DMA Engine support
+  depends on (PCI  X86) || ARCH_IOP32X || ARCH_IOP33X 
|| ARCH_IOP13XX || AVR32
   help
-Intel(R) offload engines enable offloading memory 
copies in the
-network stack and RAID operations in the MD driver.
+DMA engines can do asynchronous data transfers without
+involving the host CPU. This can be used to offload memory
+copies in the network stack and RAID operations in the MD
+driver.
 
 if DMADEVICES
 
-- 
1.5.3.4


Acked-by: Shannon Nelson [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] Remove bogus default y for DMAR and NET_DMA

[Nelson, Shannon]

From: Jeff Garzik [mailto:[EMAIL PROTECTED] 

Andi Kleen wrote:
 Index: linux-2.6.24-rc1-hack/arch/x86_64/Kconfig
 ===
 --- linux-2.6.24-rc1-hack.orig/arch/x86_64/Kconfig
 +++ linux-2.6.24-rc1-hack/arch/x86_64/Kconfig
 @@ -753,7 +753,6 @@ config PCI_DOMAINS
  config DMAR
  bool Support for DMA Remapping Devices (EXPERIMENTAL)
  depends on PCI_MSI  ACPI  EXPERIMENTAL
 -default y
  help
DMA remapping (DMAR) devices support enables 
independent address
translations for Direct Memory Access (DMA) from devices.

ACK


 Index: linux-2.6.24-rc1-hack/drivers/dma/Kconfig
 ===
 --- linux-2.6.24-rc1-hack.orig/drivers/dma/Kconfig
 +++ linux-2.6.24-rc1-hack/drivers/dma/Kconfig
 @@ -43,7 +43,6 @@ comment DMA Clients
  config NET_DMA
  bool Network: TCP receive copy offload
  depends on DMA_ENGINE  NET
 -default y
  help
This enables the use of DMA engines in the network stack to
offload receive copy-to-user operations, freeing CPU cycles.

ACK -- but its arguable that given the current code, its worth setting 
this if DMA_ENGINE is also set.


The intent is to give us a DMA offload for NIC-to-user operations.  At
the moment the ioatdma device is the only gizmo supporting this through
NET_DMA, and is the only DMA_ENGINE supported on x86.  As more devices
are made available through DMA_ENGINE on platforms that don't have
ioatdma, removing the default y makes sense.  I agree that this is
ACK-able, but under the it should just work philosophy, and with the
Intel servers coming out with the hardware support, it may want to wait
until there are more DMA_ENGINE choices.

In any case, I'll ACK this and let the maintainers decide on the timing.

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] I/OAT: Add support for version 2 of ioatdma device

[Nelson, Shannon]

>From: Andrew Morton [mailto:[EMAIL PROTECTED] 
>On Fri, 19 Oct 2007 00:20:25 -0700
>Shannon Nelson <[EMAIL PROTECTED]> wrote:
>
>> Add support for version 2 of the ioatdma device.  This device handles
>> the descriptor chain and DCA services slightly differently:
>>  - Instead of moving the dma descriptors between a busy and 
>an idle chain,
>>this new version uses a single circular chain so that we 
>don't have
>>rewrite the next_descriptor pointers as we add new 
>requests, and the
>>device doesn't need to re-read the last descriptor.
>>  - The new device has the DCA tags defined internally 
>instead of needing
>>them defined statically.
>> 

Andrew,

Thank you for your time and the review comments.  I'll attend to the
details and repost.

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] I/OAT: Add support for version 2 of ioatdma device

[Nelson, Shannon]

From: Andrew Morton [mailto:[EMAIL PROTECTED] 
On Fri, 19 Oct 2007 00:20:25 -0700
Shannon Nelson [EMAIL PROTECTED] wrote:

 Add support for version 2 of the ioatdma device.  This device handles
 the descriptor chain and DCA services slightly differently:
  - Instead of moving the dma descriptors between a busy and 
an idle chain,
this new version uses a single circular chain so that we 
don't have
rewrite the next_descriptor pointers as we add new 
requests, and the
device doesn't need to re-read the last descriptor.
  - The new device has the DCA tags defined internally 
instead of needing
them defined statically.
 

Andrew,

Thank you for your time and the review comments.  I'll attend to the
details and repost.

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] I/OAT: Add completion callback for async_tx interface use

[Nelson, Shannon]

>From: David Miller [mailto:[EMAIL PROTECTED] 
>
>From: Andrew Morton <[EMAIL PROTECTED]>
>Date: Wed, 17 Oct 2007 17:44:38 -0700
>
>> On Wed, 17 Oct 2007 17:14:39 -0700
>> Shannon Nelson <[EMAIL PROTECTED]> wrote:
>> 
>> > +  tx->callback = (void *)ioat_dma_test_callback;
>> 
>> and when I remove this cast I get
>> 
>> drivers/dma/ioat_dma.c: In function 'ioat_dma_self_test':
>> drivers/dma/ioat_dma.c:718: warning: assignment from 
>incompatible pointer type
>> 
>> because ioat_dma_test_callback isn't void-returning.  
>Something is wrong
>> here.  I assume that ioat_dma_test_callback() should just be
>> void-returning?
>
>In fact this might crash on systems that conditionally pop
>return values based upon whether the function is void or not.
>

Yep - thanks.  That's my misuse of Dan's dma_async_tx_callback typedef.

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] I/OAT: Add completion callback for async_tx interface use

[Nelson, Shannon]

>From: Andrew Morton [mailto:[EMAIL PROTECTED] 
>
>On Wed, 17 Oct 2007 17:14:39 -0700
>Shannon Nelson <[EMAIL PROTECTED]> wrote:
[...]
>> +static dma_async_tx_callback ioat_dma_test_callback(void 
>*dma_async_param)
>> +{
>> +printk(KERN_ERR "ioatdma: ioat_dma_test_callback(0x%04llx)\n",
>> +(u64)dma_async_param);
>> +return 0;
>> +}
>
>This wanted to be `return NULL'.  I'll fix.

Thanks.

>
>>  /**
>>   * ioat_dma_self_test - Perform a IOAT transaction to 
>verify the HW works.
>>   * @device: device to be tested
>> @@ -691,6 +715,8 @@ static int ioat_dma_self_test(struct 
>ioatdma_device *device)
>>  addr = dma_map_single(dma_chan->device->dev, dest, 
>IOAT_TEST_SIZE,
>>  DMA_FROM_DEVICE);
>>  ioat_set_dest(addr, tx, 0);
>> +tx->callback = (void *)ioat_dma_test_callback;
>
>This cast is unneeded, surely?  It had better be..
>
>> +tx->callback_param = (void *)0x8086;
>
>eh?

I suppose I could have used "42"...

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 4/5] I/OAT: Tighten descriptor setup performance

[Nelson, Shannon]

>-Original Message-
>From: Andrew Morton [mailto:[EMAIL PROTECTED] 
>
>On Wed, 17 Oct 2007 17:14:33 -0700
>Shannon Nelson <[EMAIL PROTECTED]> wrote:
>
>> The change to the async_tx interface cost this driver some 
>performance by
>> spreading the descriptor setup across several functions, 
>including multiple
>> passes over the new descriptor chain.  Here we bring the 
>work back into one
>> primary function and only do one pass.
>> 
>> Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
>> ---
>> 
>>  drivers/dma/ioat_dma.c |  170 
>+---
>>  drivers/dma/ioatdma.h  |6 +-
>>  2 files changed, 93 insertions(+), 83 deletions(-)
>> 
>> diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
>> index c44f551..117ac38 100644
>> --- a/drivers/dma/ioat_dma.c
>> +++ b/drivers/dma/ioat_dma.c
>> @@ -46,9 +46,12 @@
>>  /* internal functions */
>>  static void ioat_dma_start_null_desc(struct ioat_dma_chan 
>*ioat_chan);
>>  static void ioat_dma_memcpy_cleanup(struct ioat_dma_chan 
>*ioat_chan);
>> +static inline struct ioat_desc_sw *ioat_dma_get_next_descriptor(
>> +  struct 
>ioat_dma_chan *ioat_chan);
>
>A forward-declared static inline is pretty weird.  Does gcc 
>actually do the
>right thing with it?

Well, I didn't inspect the opcodes generated...

>
>This function is far too large to be inlined anyway.
>

No prob, I can remove the "inline" and repost.

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 4/5] I/OAT: Tighten descriptor setup performance

[Nelson, Shannon]

-Original Message-
From: Andrew Morton [mailto:[EMAIL PROTECTED] 

On Wed, 17 Oct 2007 17:14:33 -0700
Shannon Nelson [EMAIL PROTECTED] wrote:

 The change to the async_tx interface cost this driver some 
performance by
 spreading the descriptor setup across several functions, 
including multiple
 passes over the new descriptor chain.  Here we bring the 
work back into one
 primary function and only do one pass.
 
 Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
 ---
 
  drivers/dma/ioat_dma.c |  170 
+---
  drivers/dma/ioatdma.h  |6 +-
  2 files changed, 93 insertions(+), 83 deletions(-)
 
 diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
 index c44f551..117ac38 100644
 --- a/drivers/dma/ioat_dma.c
 +++ b/drivers/dma/ioat_dma.c
 @@ -46,9 +46,12 @@
  /* internal functions */
  static void ioat_dma_start_null_desc(struct ioat_dma_chan 
*ioat_chan);
  static void ioat_dma_memcpy_cleanup(struct ioat_dma_chan 
*ioat_chan);
 +static inline struct ioat_desc_sw *ioat_dma_get_next_descriptor(
 +  struct 
ioat_dma_chan *ioat_chan);

A forward-declared static inline is pretty weird.  Does gcc 
actually do the
right thing with it?

Well, I didn't inspect the opcodes generated...


This function is far too large to be inlined anyway.


No prob, I can remove the inline and repost.

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] I/OAT: Add completion callback for async_tx interface use

[Nelson, Shannon]

From: Andrew Morton [mailto:[EMAIL PROTECTED] 

On Wed, 17 Oct 2007 17:14:39 -0700
Shannon Nelson [EMAIL PROTECTED] wrote:
[...]
 +static dma_async_tx_callback ioat_dma_test_callback(void 
*dma_async_param)
 +{
 +printk(KERN_ERR ioatdma: ioat_dma_test_callback(0x%04llx)\n,
 +(u64)dma_async_param);
 +return 0;
 +}

This wanted to be `return NULL'.  I'll fix.

Thanks.


  /**
   * ioat_dma_self_test - Perform a IOAT transaction to 
verify the HW works.
   * @device: device to be tested
 @@ -691,6 +715,8 @@ static int ioat_dma_self_test(struct 
ioatdma_device *device)
  addr = dma_map_single(dma_chan-device-dev, dest, 
IOAT_TEST_SIZE,
  DMA_FROM_DEVICE);
  ioat_set_dest(addr, tx, 0);
 +tx-callback = (void *)ioat_dma_test_callback;

This cast is unneeded, surely?  It had better be..

 +tx-callback_param = (void *)0x8086;

eh?

I suppose I could have used 42...

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] I/OAT: Add completion callback for async_tx interface use

[Nelson, Shannon]

From: David Miller [mailto:[EMAIL PROTECTED] 

From: Andrew Morton [EMAIL PROTECTED]
Date: Wed, 17 Oct 2007 17:44:38 -0700

 On Wed, 17 Oct 2007 17:14:39 -0700
 Shannon Nelson [EMAIL PROTECTED] wrote:
 
  +  tx-callback = (void *)ioat_dma_test_callback;
 
 and when I remove this cast I get
 
 drivers/dma/ioat_dma.c: In function 'ioat_dma_self_test':
 drivers/dma/ioat_dma.c:718: warning: assignment from 
incompatible pointer type
 
 because ioat_dma_test_callback isn't void-returning.  
Something is wrong
 here.  I assume that ioat_dma_test_callback() should just be
 void-returning?

In fact this might crash on systems that conditionally pop
return values based upon whether the function is void or not.


Yep - thanks.  That's my misuse of Dan's dma_async_tx_callback typedef.

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] drivers/pci, drivers/dma: kill unused vars

[Nelson, Shannon]

>-Original Message-
>From: Jeff Garzik [mailto:[EMAIL PROTECTED] 
>Sent: Saturday, October 13, 2007 12:07 PM
>To: Andrew Morton; Linus Torvalds
>Cc: LKML; Greg KH; Nelson, Shannon; Williams, Dan J
>Subject: [PATCH] drivers/pci, drivers/dma: kill unused vars
>
>
>Kill two never-used (not even in hidden debug macros) variables,
>noticed by the compiler.
>
>Signed-off-by: Jeff Garzik <[EMAIL PROTECTED]>
>---
> drivers/dma/ioatdma.c  |1 -
> drivers/pci/hotplug/pci_hotplug_core.c |2 --
> 2 files changed, 3 deletions(-)
>
>diff --git a/drivers/dma/ioatdma.c b/drivers/dma/ioatdma.c
>index 41b18c5..d9db64b 100644
>--- a/drivers/dma/ioatdma.c
>+++ b/drivers/dma/ioatdma.c
>@@ -244,7 +244,6 @@ static void 
>ioat_dma_free_chan_resources(struct dma_chan *chan)
>   struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
>   struct ioat_device *ioat_device = to_ioat_device(chan->device);
>   struct ioat_desc_sw *desc, *_desc;
>-  u16 chanctrl;
>   int in_use_descs = 0;
> 
>   ioat_dma_memcpy_cleanup(ioat_chan);

Acked-by: Shannon Nelson <[EMAIL PROTECTED]>

FYI - this code gets heavily revised by ioat patches in -mm that Andrew
is planning on merging soon.

sln

>diff --git a/drivers/pci/hotplug/pci_hotplug_core.c 
>b/drivers/pci/hotplug/pci_hotplug_core.c
>index f0eba53..01c351c 100644
>--- a/drivers/pci/hotplug/pci_hotplug_core.c
>+++ b/drivers/pci/hotplug/pci_hotplug_core.c
>@@ -689,8 +689,6 @@ int pci_hp_deregister (struct hotplug_slot *slot)
> int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot,
>struct hotplug_slot_info *info)
> {
>-  int retval;
>-
>   if ((slot == NULL) || (info == NULL))
>   return -ENODEV;
> 
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] drivers/pci, drivers/dma: kill unused vars

[Nelson, Shannon]

-Original Message-
From: Jeff Garzik [mailto:[EMAIL PROTECTED] 
Sent: Saturday, October 13, 2007 12:07 PM
To: Andrew Morton; Linus Torvalds
Cc: LKML; Greg KH; Nelson, Shannon; Williams, Dan J
Subject: [PATCH] drivers/pci, drivers/dma: kill unused vars


Kill two never-used (not even in hidden debug macros) variables,
noticed by the compiler.

Signed-off-by: Jeff Garzik [EMAIL PROTECTED]
---
 drivers/dma/ioatdma.c  |1 -
 drivers/pci/hotplug/pci_hotplug_core.c |2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/dma/ioatdma.c b/drivers/dma/ioatdma.c
index 41b18c5..d9db64b 100644
--- a/drivers/dma/ioatdma.c
+++ b/drivers/dma/ioatdma.c
@@ -244,7 +244,6 @@ static void 
ioat_dma_free_chan_resources(struct dma_chan *chan)
   struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
   struct ioat_device *ioat_device = to_ioat_device(chan-device);
   struct ioat_desc_sw *desc, *_desc;
-  u16 chanctrl;
   int in_use_descs = 0;
 
   ioat_dma_memcpy_cleanup(ioat_chan);

Acked-by: Shannon Nelson [EMAIL PROTECTED]

FYI - this code gets heavily revised by ioat patches in -mm that Andrew
is planning on merging soon.

sln

diff --git a/drivers/pci/hotplug/pci_hotplug_core.c 
b/drivers/pci/hotplug/pci_hotplug_core.c
index f0eba53..01c351c 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -689,8 +689,6 @@ int pci_hp_deregister (struct hotplug_slot *slot)
 int __must_check pci_hp_change_slot_info(struct hotplug_slot *slot,
struct hotplug_slot_info *info)
 {
-  int retval;
-
   if ((slot == NULL) || (info == NULL))
   return -ENODEV;
 

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation and developernotes

[Nelson, Shannon]

>From: Williams, Dan J 
>Sent: Thursday, September 20, 2007 6:28 PM
>To: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; 
>[EMAIL PROTECTED]
>Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; 
>[EMAIL PROTECTED]; Nelson, Shannon
>Subject: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation 
>and developernotes
>
>Signed-off-by: Dan Williams <[EMAIL PROTECTED]>
>---
>
> Documentation/crypto/async-tx-api.txt |  217 
>+
> 1 files changed, 217 insertions(+), 0 deletions(-)

Thanks, Dan.  In addition to Randy's most excellent nit-picking, here
are a couple more minor nits.

[...]
>+4.2 "My application needs finer control of hardware channels"
>+This requirement seems to arise from cases where a DMA engine 
>driver is
[...]
>+dma_async_client_chan_request triggers dmaeninge to notify 
>the client of

s/dmaeninge/dmaengine/

[...]
>+5 SOURCE
>+drivers/dma/dmaengine.c: offload engine channel management routines
>+drivers/dma/: location for offload engine drivers
>+crypto/async_tx/async_tx.c: async_tx interface to dmaengine 
>and common code
>+crypto/async_tx/async_memcpy.c: copy offload
>+crypto/async_tx/async_memset.c: memory fill offload
>+crypto/async_tx/async_xor.c: xor offload

Also include/linux/dmaengine.h


sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation and developernotes

[Nelson, Shannon]

From: Williams, Dan J 
Sent: Thursday, September 20, 2007 6:28 PM
To: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; 
[EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; 
[EMAIL PROTECTED]; Nelson, Shannon
Subject: [PATCH 2.6.23-rc7 1/3] async_tx: usage documentation 
and developernotes

Signed-off-by: Dan Williams [EMAIL PROTECTED]
---

 Documentation/crypto/async-tx-api.txt |  217 
+
 1 files changed, 217 insertions(+), 0 deletions(-)

Thanks, Dan.  In addition to Randy's most excellent nit-picking, here
are a couple more minor nits.

[...]
+4.2 My application needs finer control of hardware channels
+This requirement seems to arise from cases where a DMA engine 
driver is
[...]
+dma_async_client_chan_request triggers dmaeninge to notify 
the client of

s/dmaeninge/dmaengine/

[...]
+5 SOURCE
+drivers/dma/dmaengine.c: offload engine channel management routines
+drivers/dma/: location for offload engine drivers
+crypto/async_tx/async_tx.c: async_tx interface to dmaengine 
and common code
+crypto/async_tx/async_memcpy.c: copy offload
+crypto/async_tx/async_memset.c: memory fill offload
+crypto/async_tx/async_xor.c: xor offload

Also include/linux/dmaengine.h


sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] Remove an unused variable from the Intel I/OAT DMA engine driver

[Nelson, Shannon]

 

>-Original Message-
>From: Jesper Juhl [mailto:[EMAIL PROTECTED] 
>Sent: Sunday, September 16, 2007 2:32 PM
>To: linux-kernel@vger.kernel.org
>Cc: Nelson, Shannon; Leech, Christopher; Jesper Juhl
>Subject: [PATCH] Remove an unused variable from the Intel 
>I/OAT DMA engine driver
>
>
>The 'u16 chanctrl' variable in 
>drivers/dma/ioatdma.c::ioat_dma_free_chan_resources() is completely 
>unused and gcc quite rightfully warns about it:
>
>  drivers/dma/ioatdma.c:247: warning: unused variable 'chanctrl'
>
>This patch removes the unused variable and silences the warning.
>
>
>Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]>
>---
>
> ioatdma.c |1 -
> 1 file changed, 1 deletion(-)
>
>--- linux-2.6/drivers/dma/ioatdma.c~   2007-09-16 
>23:24:20.0 +0200
>+++ linux-2.6/drivers/dma/ioatdma.c2007-09-16 
>23:24:20.0 +0200
>@@ -244,7 +244,6 @@ static void ioat_dma_free_chan_resources
>   struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
>   struct ioat_device *ioat_device = to_ioat_device(chan->device);
>   struct ioat_desc_sw *desc, *_desc;
>-  u16 chanctrl;
>   int in_use_descs = 0;
> 
>   ioat_dma_memcpy_cleanup(ioat_chan);
>

Yep, thanks.  That's actually used in an internal version of the driver
and got missed in the stipping.  I'll be sure it is properly taken care
of in the future.

Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] Remove an unused variable from the Intel I/OAT DMA engine driver

[Nelson, Shannon]

 

-Original Message-
From: Jesper Juhl [mailto:[EMAIL PROTECTED] 
Sent: Sunday, September 16, 2007 2:32 PM
To: linux-kernel@vger.kernel.org
Cc: Nelson, Shannon; Leech, Christopher; Jesper Juhl
Subject: [PATCH] Remove an unused variable from the Intel 
I/OAT DMA engine driver


The 'u16 chanctrl' variable in 
drivers/dma/ioatdma.c::ioat_dma_free_chan_resources() is completely 
unused and gcc quite rightfully warns about it:

  drivers/dma/ioatdma.c:247: warning: unused variable 'chanctrl'

This patch removes the unused variable and silences the warning.


Signed-off-by: Jesper Juhl [EMAIL PROTECTED]
---

 ioatdma.c |1 -
 1 file changed, 1 deletion(-)

--- linux-2.6/drivers/dma/ioatdma.c~   2007-09-16 
23:24:20.0 +0200
+++ linux-2.6/drivers/dma/ioatdma.c2007-09-16 
23:24:20.0 +0200
@@ -244,7 +244,6 @@ static void ioat_dma_free_chan_resources
   struct ioat_dma_chan *ioat_chan = to_ioat_chan(chan);
   struct ioat_device *ioat_device = to_ioat_device(chan-device);
   struct ioat_desc_sw *desc, *_desc;
-  u16 chanctrl;
   int in_use_descs = 0;
 
   ioat_dma_memcpy_cleanup(ioat_chan);


Yep, thanks.  That's actually used in an internal version of the driver
and got missed in the stipping.  I'll be sure it is properly taken care
of in the future.

Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xx processors.

[Nelson, Shannon]

>From: Scott Wood [mailto:[EMAIL PROTECTED] 
>Sent: Tuesday, September 11, 2007 7:20 AM
>To: Zhang Wei-r63237
>Cc: Nelson, Shannon; [EMAIL PROTECTED]; 
>[EMAIL PROTECTED]; Williams, Dan J; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 5/5] Add DMA engine driver for Freescale 
>MPC85xx processors.
>
>On Tue, Sep 11, 2007 at 06:10:53PM +0800, Zhang Wei-r63237 wrote:
>> > >+
>> > >+ fsl_dma_memcpy_issue_pending(chan);
>> > >+ while (fsl_dma_is_complete(chan, cookie, NULL, NULL)
>> > >+ != DMA_SUCCESS);
>> > 
>> > Again, is it possible to hang your thread here?
>> > 
>> > [...]
>> 
>> I'll add msleep here.
>
>I think what was being sought was a timout, causing the test to return
>failure.
>
>-Scott
>

Either a timeout to stop the polling, or msleep() followed by a single
call to fsl_dma_is_complete().  However, using the msleep() method is
likely to be kinder to the rest of the kernel than polling for very
long.

sln
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xx processors.

[Nelson, Shannon]

From: Scott Wood [mailto:[EMAIL PROTECTED] 
Sent: Tuesday, September 11, 2007 7:20 AM
To: Zhang Wei-r63237
Cc: Nelson, Shannon; [EMAIL PROTECTED]; 
[EMAIL PROTECTED]; Williams, Dan J; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/5] Add DMA engine driver for Freescale 
MPC85xx processors.

On Tue, Sep 11, 2007 at 06:10:53PM +0800, Zhang Wei-r63237 wrote:
  +
  + fsl_dma_memcpy_issue_pending(chan);
  + while (fsl_dma_is_complete(chan, cookie, NULL, NULL)
  + != DMA_SUCCESS);
  
  Again, is it possible to hang your thread here?
  
  [...]
 
 I'll add msleep here.

I think what was being sought was a timout, causing the test to return
failure.

-Scott


Either a timeout to stop the polling, or msleep() followed by a single
call to fsl_dma_is_complete().  However, using the msleep() method is
likely to be kinder to the rest of the kernel than polling for very
long.

sln
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

FW: [-mm patch] drivers/dma/ioat_dma.c: make 3 functions static

[Nelson, Shannon]

> From: Adrian Bunk [mailto:[EMAIL PROTECTED] 
> Sent: Sunday, September 09, 2007 1:25 PM
> To: Andrew Morton; Nelson, Shannon; Williams, Dan J
> Cc: linux-kernel@vger.kernel.org
> Subject: [-mm patch] drivers/dma/ioat_dma.c: make 3 functions static
> 

Thanks Adrian,
sln

>On Fri, Aug 31, 2007 at 09:58:22PM -0700, Andrew Morton wrote:
>>...
>> Changes since 2.6.23-rc3-mm1:
>>...
>> +i-oat-add-support-for-msi-and-msi-x.patch
>>...
>>  ioat tree
>>...

This patch makes three needlessly global functions static.

Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]>
Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>

---

 drivers/dma/ioat_dma.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

c633b44cd60648f456a11bb38fd9193ce4d6acdc 
diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
index e4c3afe..66c5bb5 100644
--- a/drivers/dma/ioat_dma.c
+++ b/drivers/dma/ioat_dma.c
@@ -47,8 +47,8 @@
 static void ioat_dma_start_null_desc(struct ioat_dma_chan *ioat_chan);
 static void ioat_dma_memcpy_cleanup(struct ioat_dma_chan *ioat_chan);
 
-struct ioat_dma_chan *ioat_lookup_chan_by_index(struct ioatdma_device
*device,
-   int index)
+static struct ioat_dma_chan *ioat_lookup_chan_by_index(struct
ioatdma_device *device,
+  int index)
 {
return device->idx[index];
 }
@@ -716,7 +716,7 @@ MODULE_PARM_DESC(ioat_interrupt_style,
  * ioat_dma_setup_interrupts - setup interrupt handler
  * @device: ioat device
  */
-int ioat_dma_setup_interrupts(struct ioatdma_device *device)
+static int ioat_dma_setup_interrupts(struct ioatdma_device *device)
 {
struct ioat_dma_chan *ioat_chan;
int err, i, j, msixcnt;
@@ -826,7 +826,7 @@ err_no_irq:
  * ioat_dma_remove_interrupts - remove whatever interrupts were set
  * @device: ioat device
  */
-void ioat_dma_remove_interrupts(struct ioatdma_device *device)
+static void ioat_dma_remove_interrupts(struct ioatdma_device *device)
 {
struct ioat_dma_chan *ioat_chan;
int i;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

FW: [-mm patch] drivers/dma/ioat_dma.c: make 3 functions static

[Nelson, Shannon]

 From: Adrian Bunk [mailto:[EMAIL PROTECTED] 
 Sent: Sunday, September 09, 2007 1:25 PM
 To: Andrew Morton; Nelson, Shannon; Williams, Dan J
 Cc: linux-kernel@vger.kernel.org
 Subject: [-mm patch] drivers/dma/ioat_dma.c: make 3 functions static
 

Thanks Adrian,
sln

On Fri, Aug 31, 2007 at 09:58:22PM -0700, Andrew Morton wrote:
...
 Changes since 2.6.23-rc3-mm1:
...
 +i-oat-add-support-for-msi-and-msi-x.patch
...
  ioat tree
...

This patch makes three needlessly global functions static.

Signed-off-by: Adrian Bunk [EMAIL PROTECTED]
Signed-off-by: Shannon Nelson [EMAIL PROTECTED]

---

 drivers/dma/ioat_dma.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

c633b44cd60648f456a11bb38fd9193ce4d6acdc 
diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
index e4c3afe..66c5bb5 100644
--- a/drivers/dma/ioat_dma.c
+++ b/drivers/dma/ioat_dma.c
@@ -47,8 +47,8 @@
 static void ioat_dma_start_null_desc(struct ioat_dma_chan *ioat_chan);
 static void ioat_dma_memcpy_cleanup(struct ioat_dma_chan *ioat_chan);
 
-struct ioat_dma_chan *ioat_lookup_chan_by_index(struct ioatdma_device
*device,
-   int index)
+static struct ioat_dma_chan *ioat_lookup_chan_by_index(struct
ioatdma_device *device,
+  int index)
 {
return device-idx[index];
 }
@@ -716,7 +716,7 @@ MODULE_PARM_DESC(ioat_interrupt_style,
  * ioat_dma_setup_interrupts - setup interrupt handler
  * @device: ioat device
  */
-int ioat_dma_setup_interrupts(struct ioatdma_device *device)
+static int ioat_dma_setup_interrupts(struct ioatdma_device *device)
 {
struct ioat_dma_chan *ioat_chan;
int err, i, j, msixcnt;
@@ -826,7 +826,7 @@ err_no_irq:
  * ioat_dma_remove_interrupts - remove whatever interrupts were set
  * @device: ioat device
  */
-void ioat_dma_remove_interrupts(struct ioatdma_device *device)
+static void ioat_dma_remove_interrupts(struct ioatdma_device *device)
 {
struct ioat_dma_chan *ioat_chan;
int i;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xx processors.

[Nelson, Shannon]

>From: Zhang Wei [mailto:[EMAIL PROTECTED] 
>Sent: Friday, September 07, 2007 3:54 AM
>To: [EMAIL PROTECTED]; Nelson, Shannon
>Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; 
>[EMAIL PROTECTED]; Zhang Wei; Ebony Zhu
>Subject: [PATCH 5/5] Add DMA engine driver for Freescale 
>MPC85xx processors.
>
>The driver implements DMA engine API for Freescale MPC85xx DMA
>controller, which could be used for MEM<-->MEM, IO_ADDR<-->MEM
>and IO_ADDR<-->IO_ADDR data transfer.
>The driver supports the Basic mode of Freescale MPC85xx DMA controller.
>The MPC85xx processors supported include MPC8540/60, MPC8555, MPC8548,
>MPC8641 and so on.
>The support for MPC83xx(MPC8349, MPC8360) is experimental.
>
>Signed-off-by: Zhang Wei <[EMAIL PROTECTED]>
>Signed-off-by: Ebony Zhu <[EMAIL PROTECTED]>
>---
> drivers/dma/Kconfig  |8 +
> drivers/dma/Makefile |1 +
> drivers/dma/fsldma.c |  995 
>++
> drivers/dma/fsldma.h |  188 ++
> 4 files changed, 1192 insertions(+), 0 deletions(-)
> create mode 100644 drivers/dma/fsldma.c
> create mode 100644 drivers/dma/fsldma.h
>
>diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>index 8f670da..a99e925 100644
>--- a/drivers/dma/Kconfig
>+++ b/drivers/dma/Kconfig
>@@ -40,4 +40,12 @@ config INTEL_IOP_ADMA
> ---help---
>   Enable support for the Intel(R) IOP Series RAID engines.
> 
>+config FSL_DMA
>+  bool "Freescale MPC85xx/MPC83xx DMA support"
>+  depends on DMA_ENGINE && PPC
>+  ---help---
>+Enable support for the Freescale DMA engine. Now, it support
>+MPC8560/40, MPC8555, MPC8548 and MPC8641 processors.
>+The MPC8349, MPC8360 support is experimental.
>+
> endmenu

If this is experimental, perhaps you should mark the depends line as
such
depends on on DMA_ENGINE && PPC && EXPERIMENTAL

[...]

>+static int fsl_dma_self_test(struct fsl_dma_chan *fsl_chan)
>+{
>+  struct dma_chan *chan;
>+  int err = 0;
>+  dma_addr_t addr;
>+  dma_cookie_t cookie;
>+  u8 *src, *dest;
>+  int i;
>+  size_t test_size;
>+  struct dma_async_tx_descriptor *tx1, *tx2, *tx3;
>+  struct fsl_dma_device *fdev;
>+
>+  BUG_ON(!fsl_chan);
>+
>+  fdev = fsl_chan->device;
>+  test_size = 4096;
>+
>+  src = kmalloc(test_size * 2, GFP_KERNEL);
>+  if (!src) {
>+  dev_err(fdev->dev,
>+  "selftest: Can not alloc memory 
>for test!\n");
>+  err = -ENOMEM;
>+  goto out;
>+  }
>+
>+  dest = src + test_size;
>+
>+  for (i = 0; i < test_size; i++)
>+  src[i] = (u8) i;
>+
>+  chan = _chan->common;
>+
>+  if (fsl_dma_alloc_chan_resources(chan) < 1) {
>+  dev_err(fdev->dev,
>+  "selftest: Can not alloc 
>resources for DMA\n");
>+  err = -ENODEV;
>+  goto out;
>+  }
>+
>+  /* TX 1 */
>+  tx1 = fsl_dma_prep_memcpy(chan, test_size / 2, 0);
>+  async_tx_ack(tx1);
>+  addr = dma_map_single(chan->device->dev, src, test_size / 2,
>+  DMA_TO_DEVICE);
>+  fsl_dma_set_src(addr, tx1, 0);
>+  addr = dma_map_single(chan->device->dev, dest, test_size / 2,
>+  
>DMA_FROM_DEVICE);
>+  fsl_dma_set_dest(addr, tx1, 0);
>+
>+  cookie = fsl_dma_tx_submit(tx1);
>+  fsl_dma_memcpy_issue_pending(chan);
>+
>+  while (fsl_dma_is_complete(chan, cookie, NULL, NULL)
>+  != DMA_SUCCESS);

Is this guaranteed to finish?  If there's something wrong and the DMA
never completes, you've now hung this thread.  This is why the ioat_dma
engine does an msleep() and then checks once for completion.  You might
think about this...

>+
>+  /* Test free and re-alloc channel resources */
>+  fsl_dma_free_chan_resources(chan);
>+
>+  if (fsl_dma_alloc_chan_resources(chan) < 1) {
>+  dev_err(fdev->dev,
>+  "selftest: Can not alloc 
>resources for DMA\n");
>+  err = -ENODEV;
>+  goto out;
>+  }
>+
>+  /* Continue to test
>+   * TX 2
>+   */
>+  tx2 = fsl_dma_prep_memcpy(chan, test_size / 4, 0);
>+  async_tx_ack(tx2);
>+  addr = dma_map_single(chan->device->dev, src + test_size / 2,
>+  test_size / 4, DMA_TO_DEVICE);
>+  fsl_dma_set_src(add

RE: [PATCH -mm] [RFC] IOAT: Add support for version 2 of ioatdma device

[Nelson, Shannon]

>From: Andy Whitcroft [mailto:[EMAIL PROTECTED] 
>
>Those two look suspect to checkpatch, and I have to agree with it.
>
>WARNING: Trailing semicolon indicates no statements, indent implies
>otherwise
>#346: FILE: drivers/dma/ioat_dma.c:168:
>+   if (--cnt == 0);
>+   break;
>WARNING: Trailing semicolon indicates no statements, indent implies
>otherwise
>#363: FILE: drivers/dma/ioat_dma.c:188:
>+   if (--cnt == 0);
>+   break;
>
>-apw
>

Ouch - how did I miss those?  Thanks!

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH -mm] [RFC] IOAT: Add support for version 2 of ioatdma device

[Nelson, Shannon]

From: Andy Whitcroft [mailto:[EMAIL PROTECTED] 

Those two look suspect to checkpatch, and I have to agree with it.

WARNING: Trailing semicolon indicates no statements, indent implies
otherwise
#346: FILE: drivers/dma/ioat_dma.c:168:
+   if (--cnt == 0);
+   break;
WARNING: Trailing semicolon indicates no statements, indent implies
otherwise
#363: FILE: drivers/dma/ioat_dma.c:188:
+   if (--cnt == 0);
+   break;

-apw


Ouch - how did I miss those?  Thanks!

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 5/5] Add DMA engine driver for Freescale MPC85xx processors.

[Nelson, Shannon]

From: Zhang Wei [mailto:[EMAIL PROTECTED] 
Sent: Friday, September 07, 2007 3:54 AM
To: [EMAIL PROTECTED]; Nelson, Shannon
Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; 
[EMAIL PROTECTED]; Zhang Wei; Ebony Zhu
Subject: [PATCH 5/5] Add DMA engine driver for Freescale 
MPC85xx processors.

The driver implements DMA engine API for Freescale MPC85xx DMA
controller, which could be used for MEM--MEM, IO_ADDR--MEM
and IO_ADDR--IO_ADDR data transfer.
The driver supports the Basic mode of Freescale MPC85xx DMA controller.
The MPC85xx processors supported include MPC8540/60, MPC8555, MPC8548,
MPC8641 and so on.
The support for MPC83xx(MPC8349, MPC8360) is experimental.

Signed-off-by: Zhang Wei [EMAIL PROTECTED]
Signed-off-by: Ebony Zhu [EMAIL PROTECTED]
---
 drivers/dma/Kconfig  |8 +
 drivers/dma/Makefile |1 +
 drivers/dma/fsldma.c |  995 
++
 drivers/dma/fsldma.h |  188 ++
 4 files changed, 1192 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dma/fsldma.c
 create mode 100644 drivers/dma/fsldma.h

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 8f670da..a99e925 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -40,4 +40,12 @@ config INTEL_IOP_ADMA
 ---help---
   Enable support for the Intel(R) IOP Series RAID engines.
 
+config FSL_DMA
+  bool Freescale MPC85xx/MPC83xx DMA support
+  depends on DMA_ENGINE  PPC
+  ---help---
+Enable support for the Freescale DMA engine. Now, it support
+MPC8560/40, MPC8555, MPC8548 and MPC8641 processors.
+The MPC8349, MPC8360 support is experimental.
+
 endmenu

If this is experimental, perhaps you should mark the depends line as
such
depends on on DMA_ENGINE  PPC  EXPERIMENTAL

[...]

+static int fsl_dma_self_test(struct fsl_dma_chan *fsl_chan)
+{
+  struct dma_chan *chan;
+  int err = 0;
+  dma_addr_t addr;
+  dma_cookie_t cookie;
+  u8 *src, *dest;
+  int i;
+  size_t test_size;
+  struct dma_async_tx_descriptor *tx1, *tx2, *tx3;
+  struct fsl_dma_device *fdev;
+
+  BUG_ON(!fsl_chan);
+
+  fdev = fsl_chan-device;
+  test_size = 4096;
+
+  src = kmalloc(test_size * 2, GFP_KERNEL);
+  if (!src) {
+  dev_err(fdev-dev,
+  selftest: Can not alloc memory 
for test!\n);
+  err = -ENOMEM;
+  goto out;
+  }
+
+  dest = src + test_size;
+
+  for (i = 0; i  test_size; i++)
+  src[i] = (u8) i;
+
+  chan = fsl_chan-common;
+
+  if (fsl_dma_alloc_chan_resources(chan)  1) {
+  dev_err(fdev-dev,
+  selftest: Can not alloc 
resources for DMA\n);
+  err = -ENODEV;
+  goto out;
+  }
+
+  /* TX 1 */
+  tx1 = fsl_dma_prep_memcpy(chan, test_size / 2, 0);
+  async_tx_ack(tx1);
+  addr = dma_map_single(chan-device-dev, src, test_size / 2,
+  DMA_TO_DEVICE);
+  fsl_dma_set_src(addr, tx1, 0);
+  addr = dma_map_single(chan-device-dev, dest, test_size / 2,
+  
DMA_FROM_DEVICE);
+  fsl_dma_set_dest(addr, tx1, 0);
+
+  cookie = fsl_dma_tx_submit(tx1);
+  fsl_dma_memcpy_issue_pending(chan);
+
+  while (fsl_dma_is_complete(chan, cookie, NULL, NULL)
+  != DMA_SUCCESS);

Is this guaranteed to finish?  If there's something wrong and the DMA
never completes, you've now hung this thread.  This is why the ioat_dma
engine does an msleep() and then checks once for completion.  You might
think about this...

+
+  /* Test free and re-alloc channel resources */
+  fsl_dma_free_chan_resources(chan);
+
+  if (fsl_dma_alloc_chan_resources(chan)  1) {
+  dev_err(fdev-dev,
+  selftest: Can not alloc 
resources for DMA\n);
+  err = -ENODEV;
+  goto out;
+  }
+
+  /* Continue to test
+   * TX 2
+   */
+  tx2 = fsl_dma_prep_memcpy(chan, test_size / 4, 0);
+  async_tx_ack(tx2);
+  addr = dma_map_single(chan-device-dev, src + test_size / 2,
+  test_size / 4, DMA_TO_DEVICE);
+  fsl_dma_set_src(addr, tx2, 0);
+  addr = dma_map_single(chan-device-dev, dest + test_size / 2,
+  test_size / 4, DMA_FROM_DEVICE);
+  fsl_dma_set_dest(addr, tx2, 0);
+
+  /* TX 3 */
+  tx3 = fsl_dma_prep_memcpy(chan, test_size / 4, 0);
+  async_tx_ack(tx3);
+  addr = dma_map_single(chan-device-dev, src + 
test_size * 3 / 4,
+  test_size / 4, DMA_TO_DEVICE);
+  fsl_dma_set_src(addr, tx3, 0);
+  addr = dma_map_single(chan-device-dev, dest + 
test_size * 3 / 4,
+  test_size / 4, DMA_FROM_DEVICE);
+  fsl_dma_set_dest(addr, tx3, 0

RE: [PATCH v2 -mm 7/7] I/OAT: Add DCA services

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 
>
>On Thu, 23 Aug 2007 17:15:27 -0700 Shannon Nelson wrote:
>
[...]
>> +
>> +/* THIS STUFF NEEDS TO LIVE SOMEWHERE ELSE */
>> +#define X86_FEATURE_DCA (4*32+18) /* Direct Cache Access */
>> +/* / THIS STUFF NEEDS TO LIVE SOMEWHERE ELSE */
>
>Yes, feature bits need to be integrated with the other CPU-specific
>feature bits.

Added to include/asm-i386/cpufeature.h, which is also used by the x86_64
version.

>
>> +/* pack PCI B/D/F into a u16 */
>> +static inline u16 dcaid_from_pcidev(struct pci_dev *pci)
>> +{
>> +return (pci->bus->number << 8) | pci->devfn;
>> +}
>
>Could any other PCI users use this in include/linux/pci.h ?

Possibly, but I don't know pci well enough to know for sure, so I'll let
someone else jmp in here.

>
>
>> diff --git a/drivers/dma/ioatdma.h b/drivers/dma/ioatdma.h
>> index 5931e56..d59c90b 100644
>> --- a/drivers/dma/ioatdma.h
>> +++ b/drivers/dma/ioatdma.h
>> @@ -132,9 +132,11 @@ struct ioat_desc_sw {
>>  #if defined(CONFIG_INTEL_IOATDMA) || 
>defined(CONFIG_INTEL_IOATDMA_MODULE)
>>  struct ioatdma_device *ioat_dma_probe(struct pci_dev *, 
>void __iomem *);
>>  void ioat_dma_remove(struct ioatdma_device *device);
>> +struct dca_provider *ioat_dca_init(struct pci_dev *, void 
>__iomem *);
>
>Parameter (variable) names in prototypes, please.

Done.

Thanks again,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 5/7] I/OAT: Add support for MSI and MSI-X

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 
>
>On Thu, 23 Aug 2007 17:15:17 -0700 Shannon Nelson wrote:
>
>> Add support for MSI and MSI-X interrupt handling, including 
>the ability
>> to choose the desired interrupt method.
>> 
>> Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
>> Acked-by: David S. Miller <[EMAIL PROTECTED]>
>> ---
>> 
>>  drivers/dma/ioat_dma.c  |  353 
>---
>>  drivers/dma/ioatdma.h   |   12 +
>>  drivers/dma/ioatdma_registers.h |6 +
>>  3 files changed, 305 insertions(+), 66 deletions(-)
>> 
>> diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
>> index 9012176..0909fee 100644
>> --- a/drivers/dma/ioat_dma.c
>> +++ b/drivers/dma/ioat_dma.c
>> @@ -47,6 +47,71 @@
>>  static void ioat_dma_start_null_desc(struct ioat_dma_chan 
>*ioat_chan);
>>  static void ioat_dma_memcpy_cleanup(struct ioat_dma_chan 
>*ioat_chan);
>>  
>> +#define for_each_bit(bit, addr, size) \
>> +for ((bit) = find_first_bit((addr), (size)); \
>> + (bit) < (size); \
>> + (bit) = find_next_bit((addr), (size), (bit) + 1))
>
>Any reason that this macro shouldn't be added to
>include/linux/bitops.h instead of here?  I'd prefer/expect such
>general-purpose macros to live somewhere else.

Sure, I can do this.  Davem had a similar comment earlier.

>
>
>> @@ -276,6 +352,34 @@ static void 
>ioat_dma_free_chan_resources(struct dma_chan *chan)
>>  in_use_descs - 1);
>>  
>>  ioat_chan->last_completion = ioat_chan->completion_addr = 0;
>> +ioat_chan->pending = 0;
>> +}
>> +/**
>> + * ioat_dma_get_next_descriptor - return the next available 
>descriptor from
>> + *the chain
>
>Unfortunately, function name + short description in kernel-doc must be
>on one line (only).  If you want to add more text/description, put it
>after the parameters (like you have done :).

Got it.

>
>> + * @ioat_chan: IOAT DMA channel handle
>
>We normally use one
> *
>line between the parameters and following text.

Okay

>
>> + * Gets the next descriptor from the chain, and must be 
>called with the
>> + * channel's desc_lock held.  Allocates more descriptors if 
>the channel
>> + * has run out.
>> + */
>> +static struct ioat_desc_sw *ioat_dma_get_next_descriptor(
>> +struct 
>ioat_dma_chan *ioat_chan)
>> +{
>> +struct ioat_desc_sw *new = NULL;
>> +
>> +if (!list_empty(_chan->free_desc)) {
>> +new = to_ioat_desc(ioat_chan->free_desc.next);
>> +list_del(>node);
>> +} else {
>> +/* try to get another desc */
>> +new = ioat_dma_alloc_descriptor(ioat_chan, GFP_ATOMIC);
>> +/* will this ever happen? */
>> +/* TODO add upper limit on these */
>> +BUG_ON(!new);
>> +}
>> +
>> +prefetch(new->hw);
>> +return new;
>>  }
>>  
>>  static struct dma_async_tx_descriptor *ioat_dma_prep_memcpy(
>> @@ -639,6 +711,162 @@ out:
>>  return err;
>>  }
>>  
>> +static char ioat_interrupt_style[32] = "msix";
>> +module_param_string(ioat_interrupt_style, ioat_interrupt_style,
>> +sizeof(ioat_interrupt_style), 0644);
>> +MODULE_PARM_DESC(ioat_interrupt_style,
>> + "set ioat interrupt style: msix (default), "
>> + "msix-single-vector, msi, intx)");
>> +
>> +/**
>> + * ioat_dma_setup_interrupts - setup interrupt handler, 
>choosing btwn msix,
>> + * msi, and legacy
>
>Same one-line nit.  legacy == intx, I suppose.

Got it.

>
>
>> + * @device: ioat device
>> + */
>> +int ioat_dma_setup_interrupts(struct ioatdma_device *device)
>> +{
>> +struct ioat_dma_chan *ioat_chan;
>> +int err, i, j, msixcnt;
>> +u8 intrctrl = 0;
>> +
>> +if (!strcmp(ioat_interrupt_style, "msix"))
>> +goto msix;
>> +else if (!strcmp(ioat_interrupt_style, "msix-single-vector"))
>> +goto msix_single_vector;
>> +else if (!strcmp(ioat_interrupt_style, "msi"))
>> +goto msi;
>> +else if (!strcmp(ioat_interrupt_style, "intx"))
>> +goto intx;
>
>The "else"s aren't needed.

Hmmm - yep.

>
>> +
>> +msix:
>
>...
>
>---
>~Randy
>*** Remember to use Documentation/SubmitChecklist when testing 
>your code ***
>

Thanks,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 4/7] I/OAT: Split PCI startup from DMA handling code

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 
>
>On Thu, 23 Aug 2007 17:15:12 -0700 Shannon Nelson wrote:
>
>> Split the general PCI startup from the DMA handling code in order to
>> prepare for adding support for DCA services and future 
>versions of the
>> ioatdma device.
>> 
>> Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
>> Acked-by: David S. Miller <[EMAIL PROTECTED]>
>> ---
>> 
>>  drivers/dma/Makefile |2 
>>  drivers/dma/ioat.c   |  186 
>
>>  drivers/dma/ioat_dma.c   |  196 
>+++---
>>  drivers/dma/ioatdma.h|   16 +++-
>>  drivers/dma/ioatdma_hw.h |2 
>>  5 files changed, 245 insertions(+), 157 deletions(-)
>
>> diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
>> index 9a4d154..9012176 100644
>> --- a/drivers/dma/ioat_dma.c
>> +++ b/drivers/dma/ioat_dma.c
>
>> -printk(KERN_INFO " "
>> -"ioatdma: Intel(R) I/OAT DMA Engine 
>initialization failed\n");
>> -
>> -return err;
>> +iounmap(iobase);
>> +printk(KERN_ERR " "
>> +   "ioatdma: Intel(R) I/OAT DMA Engine 
>initialization failed\n");
>
>Drop the " " after KERN_ERR...

Yes, is dumb, but is addressed in the next patch in the series.

>
>> +return NULL;
>>  }
>
>> diff --git a/drivers/dma/ioatdma.h b/drivers/dma/ioatdma.h
>> index bf4dad7..26aff08 100644
>> --- a/drivers/dma/ioatdma.h
>> +++ b/drivers/dma/ioatdma.h
>> @@ -31,7 +31,7 @@
>>  #define IOAT_LOW_COMPLETION_MASK0xffc0
>>  
>>  /**
>> - * struct ioat_device - internal representation of a IOAT device
>> + * struct ioatdma_device - internal representation of a IOAT device
>>   * @pdev: PCI-Express device
>>   * @reg_base: MMIO register space base address
>>   * @dma_pool: for allocating DMA descriptors
>> @@ -39,14 +39,14 @@
>>   * @msi: Message Signaled Interrupt number
>>   */
>>  
>> -struct ioat_device {
>> +struct ioatdma_device {
>>  struct pci_dev *pdev;
>>  void __iomem *reg_base;
>>  struct pci_pool *dma_pool;
>>  struct pci_pool *completion_pool;
>>  
>>  struct dma_device common;
>> -u8 msi;
>> +u8 version;
>
>This field name change needs a corresponding change in the
>struct's kernel-doc above here.

Oops, missed that one - thanks.

>
>>  };
>>  
>>  /**
>
>> @@ -117,4 +117,12 @@ struct ioat_desc_sw {
>>  struct dma_async_tx_descriptor async_tx;
>>  };
>>  
>> +#if defined(CONFIG_INTEL_IOATDMA) || 
>defined(CONFIG_INTEL_IOATDMA_MODULE)
>> +struct ioatdma_device *ioat_dma_probe(struct pci_dev *, 
>void __iomem *);
>
>Please use parameter variable names in function prototypes (above;
>most places already have them).

Yep, I'll get it.

Again, thanks for your comments,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 6/7] DCA: Add Direct Cache Access driver

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 
>
>On Thu, 23 Aug 2007 17:15:22 -0700 Shannon Nelson wrote:
>
>> Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
>> Acked-by: David S. Miller <[EMAIL PROTECTED]>
>> ---
>> 
>>  drivers/Kconfig |2 +
>>  drivers/Makefile|1 
>>  drivers/dca/Kconfig |   11 +++
>>  drivers/dca/Makefile|2 +
>>  drivers/dca/dca-core.c  |  168 
>+++
>>  drivers/dca/dca-sysfs.c |   88 +
>>  include/linux/dca.h |   47 +
>>  7 files changed, 319 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/dca/Kconfig b/drivers/dca/Kconfig
>> new file mode 100644
>> index 000..d901615
>> --- /dev/null
>> +++ b/drivers/dca/Kconfig
>> @@ -0,0 +1,11 @@
>> +#
>> +# DCA server configuration
>> +#
>> +
>> +config DCA
>> +tristate "DCA support for clients and providers"
>> +---help---
>> +  This is a server to help modules that want to use 
>Direct Cache
>> +  Access to find DCA providers that will supply correct 
>CPU tags.
>> +default m
>
>We conventionally put help text last in each config entry.
>& Help text should be indented by 1 tab + 2 spaces.
>
>> diff --git a/drivers/dca/dca-core.c b/drivers/dca/dca-core.c
>> new file mode 100644
>> index 000..c0ff9bd
>> --- /dev/null
>> +++ b/drivers/dca/dca-core.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * Copyright(c) 2007 Intel Corporation. All rights reserved.
>> + *
>> +/*
>> + * This driver supports an interface for DCA clients and 
>providers to meet.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +MODULE_LICENSE("GPL");
>> +
>> +/* For now we're assuming a single, global, DCA provider 
>for the system. */
>> +
>> +static DEFINE_SPINLOCK(dca_lock);
>> +
>> +struct dca_provider *global_dca = NULL;
>
>Can global_dca be static, or is it used in other source files?

Yes, this should be static.  I'll fix this.

>
>It would be good to have all of these global/exported interfaces
>documented somewhere.  Did I miss it in another file?
>If not, you could use kernel-doc to add inline function docs.
>See Documentation/kernel-doc-nano-HOWTO.txt.

I'll add the block comments.

>
>> +u8 dca_get_tag(int cpu)
>> +{
>> +if (!global_dca)
>> +return -ENODEV;
>> +return global_dca->ops->get_tag(global_dca, cpu);
>> +}
>> +EXPORT_SYMBOL(dca_get_tag);
>> +
>
>> +
>> +static BLOCKING_NOTIFIER_HEAD(dca_provider_chain);
>> +
>> +
>> +static int __init dca_init(void)
>> +{
>> +int err;
>> +
>> +err = dca_sysfs_init();
>> +if (err)
>> +return err;
>> +return 0;
>
>or just (in all cases):
>
>   return err;
>
>> +}

Yep, will do.

Thanks for the comments,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 3/7] I/OAT: code cleanup from checkpatch output

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 
>Sent: Thursday, August 23, 2007 10:39 PM
>To: Nelson, Shannon
>Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org; 
>[EMAIL PROTECTED]
>Subject: Re: [PATCH v2 -mm 3/7] I/OAT: code cleanup from 
>checkpatch output
>
>On Thu, 23 Aug 2007 17:15:06 -0700 Shannon Nelson wrote:
>
>> Take care of a bunch of little code nits in ioatdma files
>> 
>> Signed-off-by: Shannon Nelson <[EMAIL PROTECTED]>
>> Acked-by: David S. Miller <[EMAIL PROTECTED]>
>> ---
>> 
>>  drivers/dma/ioat_dma.c |  200 
>+++-
>>  1 files changed, 111 insertions(+), 89 deletions(-)
>> 
>> diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
>> index 55227d4..9a4d154 100644
>> --- a/drivers/dma/ioat_dma.c
>> +++ b/drivers/dma/ioat_dma.c
>
>> -printk(KERN_INFO "Intel(R) I/OAT DMA Engine found, %d
channels\n",
>> -device->common.chancnt);
>> +printk(KERN_INFO " "
>> + "ioatdma: Intel(R) I/OAT DMA Engine found, %d
channels\n",
>> + device->common.chancnt);
>>  
>>  err = ioat_self_test(device);
>>  if (err)
>
>> @@ -764,7 +784,8 @@ err_set_dma_mask:
>>  pci_disable_device(pdev);
>>  err_enable_device:
>>  
>> -printk(KERN_ERR "Intel(R) I/OAT DMA Engine initialization
failed\n");
>> +printk(KERN_INFO " "
>> +"ioatdma: Intel(R) I/OAT DMA Engine initialization
failed\n");
>>  
>>  return err;
>>  }
>
>What's with these (KERN_INFO " "
>   "...more strings");
>??

These are admittedly not the smartest move, but they are replaced later
in the patch-set.
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 3/7] I/OAT: code cleanup from checkpatch output

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 
Sent: Thursday, August 23, 2007 10:39 PM
To: Nelson, Shannon
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org; 
[EMAIL PROTECTED]
Subject: Re: [PATCH v2 -mm 3/7] I/OAT: code cleanup from 
checkpatch output

On Thu, 23 Aug 2007 17:15:06 -0700 Shannon Nelson wrote:

 Take care of a bunch of little code nits in ioatdma files
 
 Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
 Acked-by: David S. Miller [EMAIL PROTECTED]
 ---
 
  drivers/dma/ioat_dma.c |  200 
+++-
  1 files changed, 111 insertions(+), 89 deletions(-)
 
 diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
 index 55227d4..9a4d154 100644
 --- a/drivers/dma/ioat_dma.c
 +++ b/drivers/dma/ioat_dma.c

 -printk(KERN_INFO Intel(R) I/OAT DMA Engine found, %d
channels\n,
 -device-common.chancnt);
 +printk(KERN_INFO  
 + ioatdma: Intel(R) I/OAT DMA Engine found, %d
channels\n,
 + device-common.chancnt);
  
  err = ioat_self_test(device);
  if (err)

 @@ -764,7 +784,8 @@ err_set_dma_mask:
  pci_disable_device(pdev);
  err_enable_device:
  
 -printk(KERN_ERR Intel(R) I/OAT DMA Engine initialization
failed\n);
 +printk(KERN_INFO  
 +ioatdma: Intel(R) I/OAT DMA Engine initialization
failed\n);
  
  return err;
  }

What's with these (KERN_INFO  
   ...more strings);
??

These are admittedly not the smartest move, but they are replaced later
in the patch-set.
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 6/7] DCA: Add Direct Cache Access driver

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 

On Thu, 23 Aug 2007 17:15:22 -0700 Shannon Nelson wrote:

 Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
 Acked-by: David S. Miller [EMAIL PROTECTED]
 ---
 
  drivers/Kconfig |2 +
  drivers/Makefile|1 
  drivers/dca/Kconfig |   11 +++
  drivers/dca/Makefile|2 +
  drivers/dca/dca-core.c  |  168 
+++
  drivers/dca/dca-sysfs.c |   88 +
  include/linux/dca.h |   47 +
  7 files changed, 319 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/dca/Kconfig b/drivers/dca/Kconfig
 new file mode 100644
 index 000..d901615
 --- /dev/null
 +++ b/drivers/dca/Kconfig
 @@ -0,0 +1,11 @@
 +#
 +# DCA server configuration
 +#
 +
 +config DCA
 +tristate DCA support for clients and providers
 +---help---
 +  This is a server to help modules that want to use 
Direct Cache
 +  Access to find DCA providers that will supply correct 
CPU tags.
 +default m

We conventionally put help text last in each config entry.
 Help text should be indented by 1 tab + 2 spaces.

 diff --git a/drivers/dca/dca-core.c b/drivers/dca/dca-core.c
 new file mode 100644
 index 000..c0ff9bd
 --- /dev/null
 +++ b/drivers/dca/dca-core.c
 @@ -0,0 +1,168 @@
 +/*
 + * Copyright(c) 2007 Intel Corporation. All rights reserved.
 + *
 +/*
 + * This driver supports an interface for DCA clients and 
providers to meet.
 + */
 +
 +#include linux/kernel.h
 +#include linux/notifier.h
 +#include linux/device.h
 +#include linux/dca.h
 +
 +MODULE_LICENSE(GPL);
 +
 +/* For now we're assuming a single, global, DCA provider 
for the system. */
 +
 +static DEFINE_SPINLOCK(dca_lock);
 +
 +struct dca_provider *global_dca = NULL;

Can global_dca be static, or is it used in other source files?

Yes, this should be static.  I'll fix this.


It would be good to have all of these global/exported interfaces
documented somewhere.  Did I miss it in another file?
If not, you could use kernel-doc to add inline function docs.
See Documentation/kernel-doc-nano-HOWTO.txt.

I'll add the block comments.


 +u8 dca_get_tag(int cpu)
 +{
 +if (!global_dca)
 +return -ENODEV;
 +return global_dca-ops-get_tag(global_dca, cpu);
 +}
 +EXPORT_SYMBOL(dca_get_tag);
 +

 +
 +static BLOCKING_NOTIFIER_HEAD(dca_provider_chain);
 +
 +
 +static int __init dca_init(void)
 +{
 +int err;
 +
 +err = dca_sysfs_init();
 +if (err)
 +return err;
 +return 0;

or just (in all cases):

   return err;

 +}

Yep, will do.

Thanks for the comments,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 4/7] I/OAT: Split PCI startup from DMA handling code

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 

On Thu, 23 Aug 2007 17:15:12 -0700 Shannon Nelson wrote:

 Split the general PCI startup from the DMA handling code in order to
 prepare for adding support for DCA services and future 
versions of the
 ioatdma device.
 
 Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
 Acked-by: David S. Miller [EMAIL PROTECTED]
 ---
 
  drivers/dma/Makefile |2 
  drivers/dma/ioat.c   |  186 

  drivers/dma/ioat_dma.c   |  196 
+++---
  drivers/dma/ioatdma.h|   16 +++-
  drivers/dma/ioatdma_hw.h |2 
  5 files changed, 245 insertions(+), 157 deletions(-)

 diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
 index 9a4d154..9012176 100644
 --- a/drivers/dma/ioat_dma.c
 +++ b/drivers/dma/ioat_dma.c

 -printk(KERN_INFO  
 -ioatdma: Intel(R) I/OAT DMA Engine 
initialization failed\n);
 -
 -return err;
 +iounmap(iobase);
 +printk(KERN_ERR  
 +   ioatdma: Intel(R) I/OAT DMA Engine 
initialization failed\n);

Drop the   after KERN_ERR...

Yes, is dumb, but is addressed in the next patch in the series.


 +return NULL;
  }

 diff --git a/drivers/dma/ioatdma.h b/drivers/dma/ioatdma.h
 index bf4dad7..26aff08 100644
 --- a/drivers/dma/ioatdma.h
 +++ b/drivers/dma/ioatdma.h
 @@ -31,7 +31,7 @@
  #define IOAT_LOW_COMPLETION_MASK0xffc0
  
  /**
 - * struct ioat_device - internal representation of a IOAT device
 + * struct ioatdma_device - internal representation of a IOAT device
   * @pdev: PCI-Express device
   * @reg_base: MMIO register space base address
   * @dma_pool: for allocating DMA descriptors
 @@ -39,14 +39,14 @@
   * @msi: Message Signaled Interrupt number
   */
  
 -struct ioat_device {
 +struct ioatdma_device {
  struct pci_dev *pdev;
  void __iomem *reg_base;
  struct pci_pool *dma_pool;
  struct pci_pool *completion_pool;
  
  struct dma_device common;
 -u8 msi;
 +u8 version;

This field name change needs a corresponding change in the
struct's kernel-doc above here.

Oops, missed that one - thanks.


  };
  
  /**

 @@ -117,4 +117,12 @@ struct ioat_desc_sw {
  struct dma_async_tx_descriptor async_tx;
  };
  
 +#if defined(CONFIG_INTEL_IOATDMA) || 
defined(CONFIG_INTEL_IOATDMA_MODULE)
 +struct ioatdma_device *ioat_dma_probe(struct pci_dev *, 
void __iomem *);

Please use parameter variable names in function prototypes (above;
most places already have them).

Yep, I'll get it.

Again, thanks for your comments,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 5/7] I/OAT: Add support for MSI and MSI-X

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 

On Thu, 23 Aug 2007 17:15:17 -0700 Shannon Nelson wrote:

 Add support for MSI and MSI-X interrupt handling, including 
the ability
 to choose the desired interrupt method.
 
 Signed-off-by: Shannon Nelson [EMAIL PROTECTED]
 Acked-by: David S. Miller [EMAIL PROTECTED]
 ---
 
  drivers/dma/ioat_dma.c  |  353 
---
  drivers/dma/ioatdma.h   |   12 +
  drivers/dma/ioatdma_registers.h |6 +
  3 files changed, 305 insertions(+), 66 deletions(-)
 
 diff --git a/drivers/dma/ioat_dma.c b/drivers/dma/ioat_dma.c
 index 9012176..0909fee 100644
 --- a/drivers/dma/ioat_dma.c
 +++ b/drivers/dma/ioat_dma.c
 @@ -47,6 +47,71 @@
  static void ioat_dma_start_null_desc(struct ioat_dma_chan 
*ioat_chan);
  static void ioat_dma_memcpy_cleanup(struct ioat_dma_chan 
*ioat_chan);
  
 +#define for_each_bit(bit, addr, size) \
 +for ((bit) = find_first_bit((addr), (size)); \
 + (bit)  (size); \
 + (bit) = find_next_bit((addr), (size), (bit) + 1))

Any reason that this macro shouldn't be added to
include/linux/bitops.h instead of here?  I'd prefer/expect such
general-purpose macros to live somewhere else.

Sure, I can do this.  Davem had a similar comment earlier.



 @@ -276,6 +352,34 @@ static void 
ioat_dma_free_chan_resources(struct dma_chan *chan)
  in_use_descs - 1);
  
  ioat_chan-last_completion = ioat_chan-completion_addr = 0;
 +ioat_chan-pending = 0;
 +}
 +/**
 + * ioat_dma_get_next_descriptor - return the next available 
descriptor from
 + *the chain

Unfortunately, function name + short description in kernel-doc must be
on one line (only).  If you want to add more text/description, put it
after the parameters (like you have done :).

Got it.


 + * @ioat_chan: IOAT DMA channel handle

We normally use one
 *
line between the parameters and following text.

Okay


 + * Gets the next descriptor from the chain, and must be 
called with the
 + * channel's desc_lock held.  Allocates more descriptors if 
the channel
 + * has run out.
 + */
 +static struct ioat_desc_sw *ioat_dma_get_next_descriptor(
 +struct 
ioat_dma_chan *ioat_chan)
 +{
 +struct ioat_desc_sw *new = NULL;
 +
 +if (!list_empty(ioat_chan-free_desc)) {
 +new = to_ioat_desc(ioat_chan-free_desc.next);
 +list_del(new-node);
 +} else {
 +/* try to get another desc */
 +new = ioat_dma_alloc_descriptor(ioat_chan, GFP_ATOMIC);
 +/* will this ever happen? */
 +/* TODO add upper limit on these */
 +BUG_ON(!new);
 +}
 +
 +prefetch(new-hw);
 +return new;
  }
  
  static struct dma_async_tx_descriptor *ioat_dma_prep_memcpy(
 @@ -639,6 +711,162 @@ out:
  return err;
  }
  
 +static char ioat_interrupt_style[32] = msix;
 +module_param_string(ioat_interrupt_style, ioat_interrupt_style,
 +sizeof(ioat_interrupt_style), 0644);
 +MODULE_PARM_DESC(ioat_interrupt_style,
 + set ioat interrupt style: msix (default), 
 + msix-single-vector, msi, intx));
 +
 +/**
 + * ioat_dma_setup_interrupts - setup interrupt handler, 
choosing btwn msix,
 + * msi, and legacy

Same one-line nit.  legacy == intx, I suppose.

Got it.



 + * @device: ioat device
 + */
 +int ioat_dma_setup_interrupts(struct ioatdma_device *device)
 +{
 +struct ioat_dma_chan *ioat_chan;
 +int err, i, j, msixcnt;
 +u8 intrctrl = 0;
 +
 +if (!strcmp(ioat_interrupt_style, msix))
 +goto msix;
 +else if (!strcmp(ioat_interrupt_style, msix-single-vector))
 +goto msix_single_vector;
 +else if (!strcmp(ioat_interrupt_style, msi))
 +goto msi;
 +else if (!strcmp(ioat_interrupt_style, intx))
 +goto intx;

The elses aren't needed.

Hmmm - yep.


 +
 +msix:

...

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing 
your code ***


Thanks,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 -mm 7/7] I/OAT: Add DCA services

[Nelson, Shannon]

Randy Dunlap [mailto:[EMAIL PROTECTED] 

On Thu, 23 Aug 2007 17:15:27 -0700 Shannon Nelson wrote:

[...]
 +
 +/* THIS STUFF NEEDS TO LIVE SOMEWHERE ELSE */
 +#define X86_FEATURE_DCA (4*32+18) /* Direct Cache Access */
 +/* / THIS STUFF NEEDS TO LIVE SOMEWHERE ELSE */

Yes, feature bits need to be integrated with the other CPU-specific
feature bits.

Added to include/asm-i386/cpufeature.h, which is also used by the x86_64
version.


 +/* pack PCI B/D/F into a u16 */
 +static inline u16 dcaid_from_pcidev(struct pci_dev *pci)
 +{
 +return (pci-bus-number  8) | pci-devfn;
 +}

Could any other PCI users use this in include/linux/pci.h ?

Possibly, but I don't know pci well enough to know for sure, so I'll let
someone else jmp in here.



 diff --git a/drivers/dma/ioatdma.h b/drivers/dma/ioatdma.h
 index 5931e56..d59c90b 100644
 --- a/drivers/dma/ioatdma.h
 +++ b/drivers/dma/ioatdma.h
 @@ -132,9 +132,11 @@ struct ioat_desc_sw {
  #if defined(CONFIG_INTEL_IOATDMA) || 
defined(CONFIG_INTEL_IOATDMA_MODULE)
  struct ioatdma_device *ioat_dma_probe(struct pci_dev *, 
void __iomem *);
  void ioat_dma_remove(struct ioatdma_device *device);
 +struct dca_provider *ioat_dca_init(struct pci_dev *, void 
__iomem *);

Parameter (variable) names in prototypes, please.

Done.

Thanks again,
sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH -mm] IOAT: fix for UP use of cpu_physical_id()

[Nelson, Shannon]

>-Original Message-
>From: H. Peter Anvin [mailto:[EMAIL PROTECTED] 
>Sent: Monday, August 20, 2007 3:27 PM
>To: Nelson, Shannon
>Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org; 
>[EMAIL PROTECTED]; [EMAIL PROTECTED]; Luck, Tony
>Subject: Re: [PATCH -mm] IOAT: fix for UP use of cpu_physical_id()
>
>Shannon Nelson wrote:
>> Make sure we can use cpu_physical_id() even when compiled for
>> uni-processor.
>> 
>> diff --git a/drivers/dma/ioat_dca.c b/drivers/dma/ioat_dca.c
>> index c3a500b..b1af49c 100644
>> --- a/drivers/dma/ioat_dca.c
>> +++ b/drivers/dma/ioat_dca.c
>> @@ -25,6 +25,14 @@
>>  #include 
>>  #include 
>>  #include 
>> +
>> +// either a kernel change is needed, or we need something 
>like this in kernel
>> +#ifndef CONFIG_SMP
>> +#include 
>> +#undef cpu_physical_id
>> +#define cpu_physical_id(cpu) (cpuid_ebx(1) >> 24)
>> +#endif
>> +
>
>This value should probably be cached.  Executing cpuid to get a static
>value seems like a bad idea.

Yeah, this whole cpu_physical_id() gizmo needs to be fixed so it is
actually usable for non-SMP configurations.  Unfortunately, I'm not the
guy to do it.

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH -mm] IOAT: fix for UP use of cpu_physical_id()

[Nelson, Shannon]

-Original Message-
From: H. Peter Anvin [mailto:[EMAIL PROTECTED] 
Sent: Monday, August 20, 2007 3:27 PM
To: Nelson, Shannon
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org; 
[EMAIL PROTECTED]; [EMAIL PROTECTED]; Luck, Tony
Subject: Re: [PATCH -mm] IOAT: fix for UP use of cpu_physical_id()

Shannon Nelson wrote:
 Make sure we can use cpu_physical_id() even when compiled for
 uni-processor.
 
 diff --git a/drivers/dma/ioat_dca.c b/drivers/dma/ioat_dca.c
 index c3a500b..b1af49c 100644
 --- a/drivers/dma/ioat_dca.c
 +++ b/drivers/dma/ioat_dca.c
 @@ -25,6 +25,14 @@
  #include linux/smp.h
  #include linux/interrupt.h
  #include linux/dca.h
 +
 +// either a kernel change is needed, or we need something 
like this in kernel
 +#ifndef CONFIG_SMP
 +#include asm/smp.h
 +#undef cpu_physical_id
 +#define cpu_physical_id(cpu) (cpuid_ebx(1)  24)
 +#endif
 +

This value should probably be cached.  Executing cpuid to get a static
value seems like a bad idea.

Yeah, this whole cpu_physical_id() gizmo needs to be fixed so it is
actually usable for non-SMP configurations.  Unfortunately, I'm not the
guy to do it.

sln
--
==
Mr. Shannon Nelson LAN Access Division, Intel Corp.
[EMAIL PROTECTED]I don't speak for Intel
(503) 712-7659Parents can't afford to be squeamish.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/