Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] netfilter: avoid array overflow in nf_register_hook
From: Eric Dumazet @ 2013-08-23 16:05 UTC (permalink / raw)
  To: Dong Fang
  Cc: pablo, kaber, kadlec, davem, netfilter-devel, netfilter, coreteam,
	netdev
In-Reply-To: <1377313495-16060-1-git-send-email-yp.fangdong@gmail.com>

On Fri, 2013-08-23 at 23:04 -0400, Dong Fang wrote:
> This patch fix the array overflow in nf_register_hook function
> 
> Signed-off-by: Dong Fang <yp.fangdong@gmail.com>
> ---
>  net/netfilter/core.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/net/netfilter/core.c b/net/netfilter/core.c
> index 2217363..819eee1 100644
> --- a/net/netfilter/core.c
> +++ b/net/netfilter/core.c
> @@ -68,8 +68,11 @@ static DEFINE_MUTEX(nf_hook_mutex);
>  int nf_register_hook(struct nf_hook_ops *reg)
>  {
>  	struct nf_hook_ops *elem;
> -	int err;
> +	int err = -EINVAL;
>  
> +	if (reg->pf >= NFPROTO_NUMPROTO || reg->hooknum >= NF_MAX_HOOKS)
> +		return err;
> +

Thats a bug in the caller.

We want a hard crash so that the poor netfilter guy who made this error
can fix the bug before sending us a buggy patch.



^ permalink raw reply

* Re: netem: the reorder discussion
From: Teco Boot @ 2013-08-23 16:18 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Stephen Hemminger, Ferry Huberts, Netem, netdev@vger.kernel.org,
	core-users@pf.itd.nrl.navy.mil
In-Reply-To: <1377273838.8828.11.camel@edumazet-glaptop>


Op 23 aug. 2013 om 18:03 heeft Eric Dumazet <eric.dumazet@gmail.com> het volgende geschreven:

> On Fri, 2013-08-23 at 17:53 +0200, Teco Boot wrote:
> 
>> Questions that pops out of of my head: What is more important,
>> understandable configuration or keep existing behavior for unmodified
>> configuration? Do we want to reorder, even with configured rate?
>> 
>> My preference: make in understandable, document well and make all
>> sensible options available.
> 
> The day 'rate' support was added, documentation became obsolete.
> 
> Its actually hard to have both rate and delay + jitters + reorders or
> not.
> 
> If you can fix this properly, patches are welcome, but I suspect you
> wanted to reintroduce the thing that made netem unusable for us.

No, for sure I do not want to make it unusable for you. Or anybody. The opposite is true. 
I don't like the "look in souce" documentation. Or parameters with hard to understand side effects. 

^ permalink raw reply

* pull request: wireless 2013-08-23
From: John W. Linville @ 2013-08-23 16:20 UTC (permalink / raw)
  To: davem; +Cc: linux-wireless, netdev, linux-kernel

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

Dave,

This is one more set of fixes intended for the 3.11 stream...

For the mac80211 bits, Johannes says:

"I have three more patches for the 3.11 stream: Felix's fix for the
fairly visible brcmsmac crash, a fix from Simon for an IBSS join bug I
found and a fix for a channel context bug in IBSS I'd introduced."

Along with those...

Sujith Manoharan makes a minor change to not use a PLL hang workaroun
for AR9550.  This one-liner fixes a couple of bugs reported in the Red Hat
bugzilla.

Helmut Schaa addresses an ath9k_htc bug that mangles frame headers
during Tx.  This fix is small, tested by the bug reported and isolated
to ath9k_htc.

Stanislaw Gruszka reverts a recent iwl4965 change that broke rfkill
notification to user space.

Please let me know if there are problems!

Thanks,

John

---

The following changes since commit 4a5a8aa6c966eafc106543bd955ae388230420e5:

  ipv4: expose IPV4_DEVCONF (2013-08-22 20:30:15 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git for-davem

for you to fetch changes up to 81ca2ff9450c2e983ee416eca17ebe6fa9c3da4f:

  Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless into for-davem (2013-08-23 11:47:48 -0400)

----------------------------------------------------------------

Felix Fietkau (1):
      mac80211: add a flag to indicate CCK support for HT clients

Helmut Schaa (1):
      ath9k_htc: Restore skb headroom when returning skb to mac80211

Johannes Berg (1):
      mac80211: add missing channel context release

John W. Linville (2):
      Merge branch 'for-john' of git://git.kernel.org/.../jberg/mac80211
      Merge branch 'master' of git://git.kernel.org/.../linville/wireless into for-davem

Simon Wunderlich (1):
      mac80211: ibss: fix ignored channel parameter

Stanislaw Gruszka (1):
      iwl4965: fix rfkill set state regression

Sujith Manoharan (1):
      ath9k: Enable PLL fix only for AR9340/AR9330

 drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 10 ++++++++
 drivers/net/wireless/ath/ath9k/init.c         |  3 ++-
 drivers/net/wireless/ath/ath9k/main.c         |  3 +--
 drivers/net/wireless/ath/carl9170/main.c      |  3 ++-
 drivers/net/wireless/iwlegacy/4965-mac.c      |  2 +-
 drivers/net/wireless/rt2x00/rt2800lib.c       |  3 ++-
 include/net/mac80211.h                        |  1 +
 net/mac80211/ibss.c                           | 34 +++++++++++++++++++++++----
 net/mac80211/rc80211_minstrel_ht.c            |  3 +++
 9 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
index e602c95..c028df7 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
@@ -448,6 +448,7 @@ static void ath9k_htc_tx_process(struct ath9k_htc_priv *priv,
 	struct ieee80211_conf *cur_conf = &priv->hw->conf;
 	bool txok;
 	int slot;
+	int hdrlen, padsize;
 
 	slot = strip_drv_header(priv, skb);
 	if (slot < 0) {
@@ -504,6 +505,15 @@ send_mac80211:
 
 	ath9k_htc_tx_clear_slot(priv, slot);
 
+	/* Remove padding before handing frame back to mac80211 */
+	hdrlen = ieee80211_get_hdrlen_from_skb(skb);
+
+	padsize = hdrlen & 3;
+	if (padsize && skb->len > hdrlen + padsize) {
+		memmove(skb->data + padsize, skb->data, hdrlen);
+		skb_pull(skb, padsize);
+	}
+
 	/* Send status to mac80211 */
 	ieee80211_tx_status(priv->hw, skb);
 }
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index 16f8b20..026a2a0 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -802,7 +802,8 @@ void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
 		IEEE80211_HW_PS_NULLFUNC_STACK |
 		IEEE80211_HW_SPECTRUM_MGMT |
 		IEEE80211_HW_REPORTS_TX_ACK_STATUS |
-		IEEE80211_HW_SUPPORTS_RC_TABLE;
+		IEEE80211_HW_SUPPORTS_RC_TABLE |
+		IEEE80211_HW_SUPPORTS_HT_CCK_RATES;
 
 	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_HT) {
 		hw->flags |= IEEE80211_HW_AMPDU_AGGREGATION;
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 1737a3e..cb5a655 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -173,8 +173,7 @@ static void ath_restart_work(struct ath_softc *sc)
 {
 	ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
 
-	if (AR_SREV_9340(sc->sc_ah) || AR_SREV_9485(sc->sc_ah) ||
-	    AR_SREV_9550(sc->sc_ah))
+	if (AR_SREV_9340(sc->sc_ah) || AR_SREV_9330(sc->sc_ah))
 		ieee80211_queue_delayed_work(sc->hw, &sc->hw_pll_work,
 				     msecs_to_jiffies(ATH_PLL_WORK_INTERVAL));
 
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index 4a33c6e..349fa22 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1860,7 +1860,8 @@ void *carl9170_alloc(size_t priv_size)
 		     IEEE80211_HW_PS_NULLFUNC_STACK |
 		     IEEE80211_HW_NEED_DTIM_BEFORE_ASSOC |
 		     IEEE80211_HW_SUPPORTS_RC_TABLE |
-		     IEEE80211_HW_SIGNAL_DBM;
+		     IEEE80211_HW_SIGNAL_DBM |
+		     IEEE80211_HW_SUPPORTS_HT_CCK_RATES;
 
 	if (!modparam_noht) {
 		/*
diff --git a/drivers/net/wireless/iwlegacy/4965-mac.c b/drivers/net/wireless/iwlegacy/4965-mac.c
index f2ed62e..7acf5ee 100644
--- a/drivers/net/wireless/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/iwlegacy/4965-mac.c
@@ -4464,9 +4464,9 @@ il4965_irq_tasklet(struct il_priv *il)
 			set_bit(S_RFKILL, &il->status);
 		} else {
 			clear_bit(S_RFKILL, &il->status);
-			wiphy_rfkill_set_hw_state(il->hw->wiphy, hw_rf_kill);
 			il_force_reset(il, true);
 		}
+		wiphy_rfkill_set_hw_state(il->hw->wiphy, hw_rf_kill);
 
 		handled |= CSR_INT_BIT_RF_KILL;
 	}
diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index 1f80ea5..1b41c8e 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -6133,7 +6133,8 @@ static int rt2800_probe_hw_mode(struct rt2x00_dev *rt2x00dev)
 	    IEEE80211_HW_SUPPORTS_PS |
 	    IEEE80211_HW_PS_NULLFUNC_STACK |
 	    IEEE80211_HW_AMPDU_AGGREGATION |
-	    IEEE80211_HW_REPORTS_TX_ACK_STATUS;
+	    IEEE80211_HW_REPORTS_TX_ACK_STATUS |
+	    IEEE80211_HW_SUPPORTS_HT_CCK_RATES;
 
 	/*
 	 * Don't set IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING for USB devices
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 5b7a3da..551ba6a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1499,6 +1499,7 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_SUPPORTS_RC_TABLE			= 1<<24,
 	IEEE80211_HW_P2P_DEV_ADDR_FOR_INTF		= 1<<25,
 	IEEE80211_HW_TIMING_BEACON_ONLY			= 1<<26,
+	IEEE80211_HW_SUPPORTS_HT_CCK_RATES		= 1<<27,
 };
 
 /**
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index ea7b9c2..2d45643 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -36,7 +36,7 @@
 
 static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 				      const u8 *bssid, const int beacon_int,
-				      struct ieee80211_channel *chan,
+				      struct cfg80211_chan_def *req_chandef,
 				      const u32 basic_rates,
 				      const u16 capability, u64 tsf,
 				      bool creator)
@@ -51,6 +51,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	u32 bss_change;
 	u8 supp_rates[IEEE80211_MAX_SUPP_RATES];
 	struct cfg80211_chan_def chandef;
+	struct ieee80211_channel *chan;
 	struct beacon_data *presp;
 	int frame_len;
 
@@ -81,7 +82,9 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 
 	sdata->drop_unencrypted = capability & WLAN_CAPABILITY_PRIVACY ? 1 : 0;
 
-	chandef = ifibss->chandef;
+	/* make a copy of the chandef, it could be modified below. */
+	chandef = *req_chandef;
+	chan = chandef.chan;
 	if (!cfg80211_reg_can_beacon(local->hw.wiphy, &chandef)) {
 		chandef.width = NL80211_CHAN_WIDTH_20;
 		chandef.center_freq1 = chan->center_freq;
@@ -259,10 +262,12 @@ static void ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	struct cfg80211_bss *cbss =
 		container_of((void *)bss, struct cfg80211_bss, priv);
 	struct ieee80211_supported_band *sband;
+	struct cfg80211_chan_def chandef;
 	u32 basic_rates;
 	int i, j;
 	u16 beacon_int = cbss->beacon_interval;
 	const struct cfg80211_bss_ies *ies;
+	enum nl80211_channel_type chan_type;
 	u64 tsf;
 
 	sdata_assert_lock(sdata);
@@ -270,6 +275,26 @@ static void ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	if (beacon_int < 10)
 		beacon_int = 10;
 
+	switch (sdata->u.ibss.chandef.width) {
+	case NL80211_CHAN_WIDTH_20_NOHT:
+	case NL80211_CHAN_WIDTH_20:
+	case NL80211_CHAN_WIDTH_40:
+		chan_type = cfg80211_get_chandef_type(&sdata->u.ibss.chandef);
+		cfg80211_chandef_create(&chandef, cbss->channel, chan_type);
+		break;
+	case NL80211_CHAN_WIDTH_5:
+	case NL80211_CHAN_WIDTH_10:
+		cfg80211_chandef_create(&chandef, cbss->channel,
+					NL80211_CHAN_WIDTH_20_NOHT);
+		chandef.width = sdata->u.ibss.chandef.width;
+		break;
+	default:
+		/* fall back to 20 MHz for unsupported modes */
+		cfg80211_chandef_create(&chandef, cbss->channel,
+					NL80211_CHAN_WIDTH_20_NOHT);
+		break;
+	}
+
 	sband = sdata->local->hw.wiphy->bands[cbss->channel->band];
 
 	basic_rates = 0;
@@ -294,7 +319,7 @@ static void ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 
 	__ieee80211_sta_join_ibss(sdata, cbss->bssid,
 				  beacon_int,
-				  cbss->channel,
+				  &chandef,
 				  basic_rates,
 				  cbss->capability,
 				  tsf, false);
@@ -736,7 +761,7 @@ static void ieee80211_sta_create_ibss(struct ieee80211_sub_if_data *sdata)
 		sdata->drop_unencrypted = 0;
 
 	__ieee80211_sta_join_ibss(sdata, bssid, sdata->vif.bss_conf.beacon_int,
-				  ifibss->chandef.chan, ifibss->basic_rates,
+				  &ifibss->chandef, ifibss->basic_rates,
 				  capability, 0, true);
 }
 
@@ -1138,6 +1163,7 @@ int ieee80211_ibss_leave(struct ieee80211_sub_if_data *sdata)
 	clear_bit(SDATA_STATE_OFFCHANNEL_BEACON_STOPPED, &sdata->state);
 	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BEACON_ENABLED |
 						BSS_CHANGED_IBSS);
+	ieee80211_vif_release_channel(sdata);
 	synchronize_rcu();
 	kfree(presp);
 
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index f5aed96..f3bbea1 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -828,6 +828,9 @@ minstrel_ht_update_cck(struct minstrel_priv *mp, struct minstrel_ht_sta *mi,
 	if (sband->band != IEEE80211_BAND_2GHZ)
 		return;
 
+	if (!(mp->hw->flags & IEEE80211_HW_SUPPORTS_HT_CCK_RATES))
+		return;
+
 	mi->cck_supported = 0;
 	mi->cck_supported_short = 0;
 	for (i = 0; i < 4; i++) {
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply related

* Re: [PATCH v4 2/5] net: ethernet: cpsw: add optional third memory region for CONTROL module
From: Daniel Mack @ 2013-08-23 16:21 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: netdev, bcousson, nsekhar, davem, ujhelyi.m, mugunthanvnm,
	vaibhav.bedia, d-gerlach, linux-arm-kernel, linux-omap,
	devicetree
In-Reply-To: <521778DB.7040402@cogentembedded.com>

On 23.08.2013 16:59, Sergei Shtylyov wrote:
> On 23-08-2013 18:16, Daniel Mack wrote:

>> +	priv->gmii_sel_reg = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(priv->gmii_sel_reg)) {
>> +		dev_err(priv->dev, "unable to map control i/o region\n");
> 
>     You didn't actually seem to heed my words about error message.

Well yes I did, but only in the check for platform_get_resource(). As
the comment says - we pass on if that memory region is not given, but if
it is given, it also has to be valid.

> And don't  you want to do:
> 
> 	res = PTR_ERR(priv->gmii_sel_reg);


Erm, of course. Sorry for that.


Daniel

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Daniel Mack @ 2013-08-23 16:30 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: netdev, bcousson, nsekhar, sergei.shtylyov, davem, ujhelyi.m,
	mugunthanvnm, vaibhav.bedia, d-gerlach, linux-arm-kernel,
	linux-omap, devicetree
In-Reply-To: <52177052.1030308@ti.com>

On 23.08.2013 16:23, Santosh Shilimkar wrote:
> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:

>> +static const struct of_device_id cpsw_of_mtable[] = {
>> +	{
>> +		.compatible	= "ti,am3352-cpsw",
>
> I didn't notice this earlier, but can't you use the IP version
> as a compatible instead of using a SOC name. Whats really SOC specific
> on this IP ? Sorry i have missed any earlier discussion on this but
> this approach doesn't seem good. Its like adding SOC checks in the
> driver subsystem.

As I already mentioned in the cover letter and in the commit message, I
just don't know which criteria makes most sense here.

On a general note, I would say that chances that this exactly IP core
with the same version number will appear on some other silicon which
doesn't support the control mode register in an AM33xx fashion, is not
necessarily negligible.

So what that new compatible string denotes is the cpsw in a version as
found on am3352 SoCs, which is actually exactly what it does.

I don't have a strong opinion here, but see your point. I just don't
have a better idea on how to treat that.


Daniel

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Sekhar Nori @ 2013-08-23 16:31 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: Daniel Mack, netdev, bcousson, sergei.shtylyov, davem, ujhelyi.m,
	mugunthanvnm, vaibhav.bedia, d-gerlach, linux-arm-kernel,
	linux-omap, devicetree
In-Reply-To: <52177052.1030308@ti.com>

On 8/23/2013 7:53 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>> In order to support features that are specific to the AM335x IP, we have
>> to add hardware types and another compatible string.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 4e5ca54..b717458 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>>  ------------------------------------------------------
>>  
>>  Required properties:
>> -- compatible		: Should be "ti,cpsw"
>> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
>> +			  "ti,am3352-cpsw" for AM3352 SoCs
>>  - reg			: physical base address and size of the cpsw
>>  			  registers map.
>>  			  An optional third memory region can be supplied if
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 7a25ff4..73c44cb6 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -155,6 +155,11 @@ do {								\
>>  		((priv->data.dual_emac) ? priv->emac_port :	\
>>  		priv->data.active_slave)
>>  
>> +enum {
>> +	CPSW_TYPE_GENERIC,
>> +	CPSW_TYPE_AM33XX
>> +};
>> +
>>  static int debug_level;
>>  module_param(debug_level, int, 0);
>>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
>> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>>  	slave->port_vlan = data->dual_emac_res_vlan;
>>  }
>>  
>> +static const struct of_device_id cpsw_of_mtable[] = {
>> +	{
>> +		.compatible	= "ti,am3352-cpsw",
> I didn't notice this earlier, but can't you use the IP version
> as a compatible instead of using a SOC name. Whats really SOC specific
> on this IP ? Sorry i have missed any earlier discussion on this but
> this approach doesn't seem good. Its like adding SOC checks in the
> driver subsystem.

The objection then should not just be to the name of compatible string,
but to handling AM335x control module register in CPSW driver. What
would be the alternate method of doing what Daniel is doing? I vaguely
remember there were attempts to develop an independent control module
driver that were abandoned.

Thanks,
Sekhar

^ permalink raw reply

* [PATCH] VMXNET3: Add support for virtual IOMMU
From: Andy King @ 2013-08-23 16:33 UTC (permalink / raw)
  To: netdev, linux-kernel, virtualization; +Cc: gregkh, davem, pv-drivers, Andy King

This patch adds support for virtual IOMMU to the vmxnet3 module.  We
switch to DMA consistent mappings for anything we pass to the device.
There were a few places where we already did this, but using pci_blah();
these have been fixed to use dma_blah(), along with all new occurrences
where we've replaced kmalloc() and friends.

Also fix two small bugs:
1) use after free of rq->buf_info in vmxnet3_rq_destroy()
2) a cpu_to_le32() that should have been a cpu_to_le64()

Acked-by: George Zhang <georgezhang@vmware.com>
Acked-by: Aditya Sarwade <asarwade@vmware.com>
Signed-off-by: Andy King <acking@vmware.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c |  211 +++++++++++++++++++++++--------------
 drivers/net/vmxnet3/vmxnet3_int.h |   10 ++-
 2 files changed, 138 insertions(+), 83 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 55a62ca..7e2788c 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -313,10 +313,10 @@ vmxnet3_unmap_tx_buf(struct vmxnet3_tx_buf_info *tbi,
 		     struct pci_dev *pdev)
 {
 	if (tbi->map_type == VMXNET3_MAP_SINGLE)
-		pci_unmap_single(pdev, tbi->dma_addr, tbi->len,
+		dma_unmap_single(&pdev->dev, tbi->dma_addr, tbi->len,
 				 PCI_DMA_TODEVICE);
 	else if (tbi->map_type == VMXNET3_MAP_PAGE)
-		pci_unmap_page(pdev, tbi->dma_addr, tbi->len,
+		dma_unmap_page(&pdev->dev, tbi->dma_addr, tbi->len,
 			       PCI_DMA_TODEVICE);
 	else
 		BUG_ON(tbi->map_type != VMXNET3_MAP_NONE);
@@ -429,25 +429,29 @@ vmxnet3_tq_destroy(struct vmxnet3_tx_queue *tq,
 		   struct vmxnet3_adapter *adapter)
 {
 	if (tq->tx_ring.base) {
-		pci_free_consistent(adapter->pdev, tq->tx_ring.size *
-				    sizeof(struct Vmxnet3_TxDesc),
-				    tq->tx_ring.base, tq->tx_ring.basePA);
+		dma_free_coherent(&adapter->pdev->dev, tq->tx_ring.size *
+				  sizeof(struct Vmxnet3_TxDesc),
+				  tq->tx_ring.base, tq->tx_ring.basePA);
 		tq->tx_ring.base = NULL;
 	}
 	if (tq->data_ring.base) {
-		pci_free_consistent(adapter->pdev, tq->data_ring.size *
-				    sizeof(struct Vmxnet3_TxDataDesc),
-				    tq->data_ring.base, tq->data_ring.basePA);
+		dma_free_coherent(&adapter->pdev->dev, tq->data_ring.size *
+				  sizeof(struct Vmxnet3_TxDataDesc),
+				  tq->data_ring.base, tq->data_ring.basePA);
 		tq->data_ring.base = NULL;
 	}
 	if (tq->comp_ring.base) {
-		pci_free_consistent(adapter->pdev, tq->comp_ring.size *
-				    sizeof(struct Vmxnet3_TxCompDesc),
-				    tq->comp_ring.base, tq->comp_ring.basePA);
+		dma_free_coherent(&adapter->pdev->dev, tq->comp_ring.size *
+				  sizeof(struct Vmxnet3_TxCompDesc),
+				  tq->comp_ring.base, tq->comp_ring.basePA);
 		tq->comp_ring.base = NULL;
 	}
-	kfree(tq->buf_info);
-	tq->buf_info = NULL;
+	if (tq->buf_info) {
+		dma_free_coherent(&adapter->pdev->dev,
+				  tq->tx_ring.size * sizeof(tq->buf_info[0]),
+				  tq->buf_info, tq->buf_info_pa);
+		tq->buf_info = NULL;
+	}
 }
 
 
@@ -496,37 +500,38 @@ static int
 vmxnet3_tq_create(struct vmxnet3_tx_queue *tq,
 		  struct vmxnet3_adapter *adapter)
 {
+	size_t sz;
+
 	BUG_ON(tq->tx_ring.base || tq->data_ring.base ||
 	       tq->comp_ring.base || tq->buf_info);
 
-	tq->tx_ring.base = pci_alloc_consistent(adapter->pdev, tq->tx_ring.size
-			   * sizeof(struct Vmxnet3_TxDesc),
-			   &tq->tx_ring.basePA);
+	tq->tx_ring.base = dma_alloc_coherent(&adapter->pdev->dev,
+			tq->tx_ring.size * sizeof(struct Vmxnet3_TxDesc),
+			&tq->tx_ring.basePA, GFP_KERNEL);
 	if (!tq->tx_ring.base) {
 		netdev_err(adapter->netdev, "failed to allocate tx ring\n");
 		goto err;
 	}
 
-	tq->data_ring.base = pci_alloc_consistent(adapter->pdev,
-			     tq->data_ring.size *
-			     sizeof(struct Vmxnet3_TxDataDesc),
-			     &tq->data_ring.basePA);
+	tq->data_ring.base = dma_alloc_coherent(&adapter->pdev->dev,
+			tq->data_ring.size * sizeof(struct Vmxnet3_TxDataDesc),
+			&tq->data_ring.basePA, GFP_KERNEL);
 	if (!tq->data_ring.base) {
 		netdev_err(adapter->netdev, "failed to allocate data ring\n");
 		goto err;
 	}
 
-	tq->comp_ring.base = pci_alloc_consistent(adapter->pdev,
-			     tq->comp_ring.size *
-			     sizeof(struct Vmxnet3_TxCompDesc),
-			     &tq->comp_ring.basePA);
+	tq->comp_ring.base = dma_alloc_coherent(&adapter->pdev->dev,
+			tq->comp_ring.size * sizeof(struct Vmxnet3_TxCompDesc),
+			&tq->comp_ring.basePA, GFP_KERNEL);
 	if (!tq->comp_ring.base) {
 		netdev_err(adapter->netdev, "failed to allocate tx comp ring\n");
 		goto err;
 	}
 
-	tq->buf_info = kcalloc(tq->tx_ring.size, sizeof(tq->buf_info[0]),
-			       GFP_KERNEL);
+	sz = tq->tx_ring.size * sizeof(tq->buf_info[0]);
+	tq->buf_info = dma_zalloc_coherent(&adapter->pdev->dev, sz,
+					   &tq->buf_info_pa, GFP_KERNEL);
 	if (!tq->buf_info)
 		goto err;
 
@@ -578,7 +583,8 @@ vmxnet3_rq_alloc_rx_buf(struct vmxnet3_rx_queue *rq, u32 ring_idx,
 					break;
 				}
 
-				rbi->dma_addr = pci_map_single(adapter->pdev,
+				rbi->dma_addr = dma_map_single(
+						&adapter->pdev->dev,
 						rbi->skb->data, rbi->len,
 						PCI_DMA_FROMDEVICE);
 			} else {
@@ -595,7 +601,8 @@ vmxnet3_rq_alloc_rx_buf(struct vmxnet3_rx_queue *rq, u32 ring_idx,
 					rq->stats.rx_buf_alloc_failure++;
 					break;
 				}
-				rbi->dma_addr = pci_map_page(adapter->pdev,
+				rbi->dma_addr = dma_map_page(
+						&adapter->pdev->dev,
 						rbi->page, 0, PAGE_SIZE,
 						PCI_DMA_FROMDEVICE);
 			} else {
@@ -705,7 +712,7 @@ vmxnet3_map_pkt(struct sk_buff *skb, struct vmxnet3_tx_ctx *ctx,
 
 		tbi = tq->buf_info + tq->tx_ring.next2fill;
 		tbi->map_type = VMXNET3_MAP_SINGLE;
-		tbi->dma_addr = pci_map_single(adapter->pdev,
+		tbi->dma_addr = dma_map_single(&adapter->pdev->dev,
 				skb->data + buf_offset, buf_size,
 				PCI_DMA_TODEVICE);
 
@@ -1221,7 +1228,8 @@ vmxnet3_rq_rx_complete(struct vmxnet3_rx_queue *rq,
 				goto rcd_done;
 			}
 
-			pci_unmap_single(adapter->pdev, rbi->dma_addr, rbi->len,
+			dma_unmap_single(&adapter->pdev->dev, rbi->dma_addr,
+					 rbi->len,
 					 PCI_DMA_FROMDEVICE);
 
 #ifdef VMXNET3_RSS
@@ -1233,7 +1241,7 @@ vmxnet3_rq_rx_complete(struct vmxnet3_rx_queue *rq,
 
 			/* Immediate refill */
 			rbi->skb = new_skb;
-			rbi->dma_addr = pci_map_single(adapter->pdev,
+			rbi->dma_addr = dma_map_single(&adapter->pdev->dev,
 						       rbi->skb->data, rbi->len,
 						       PCI_DMA_FROMDEVICE);
 			rxd->addr = cpu_to_le64(rbi->dma_addr);
@@ -1267,7 +1275,7 @@ vmxnet3_rq_rx_complete(struct vmxnet3_rx_queue *rq,
 			}
 
 			if (rcd->len) {
-				pci_unmap_page(adapter->pdev,
+				dma_unmap_page(&adapter->pdev->dev,
 					       rbi->dma_addr, rbi->len,
 					       PCI_DMA_FROMDEVICE);
 
@@ -1276,7 +1284,8 @@ vmxnet3_rq_rx_complete(struct vmxnet3_rx_queue *rq,
 
 			/* Immediate refill */
 			rbi->page = new_page;
-			rbi->dma_addr = pci_map_page(adapter->pdev, rbi->page,
+			rbi->dma_addr = dma_map_page(&adapter->pdev->dev,
+						     rbi->page,
 						     0, PAGE_SIZE,
 						     PCI_DMA_FROMDEVICE);
 			rxd->addr = cpu_to_le64(rbi->dma_addr);
@@ -1352,13 +1361,13 @@ vmxnet3_rq_cleanup(struct vmxnet3_rx_queue *rq,
 
 			if (rxd->btype == VMXNET3_RXD_BTYPE_HEAD &&
 					rq->buf_info[ring_idx][i].skb) {
-				pci_unmap_single(adapter->pdev, rxd->addr,
+				dma_unmap_single(&adapter->pdev->dev, rxd->addr,
 						 rxd->len, PCI_DMA_FROMDEVICE);
 				dev_kfree_skb(rq->buf_info[ring_idx][i].skb);
 				rq->buf_info[ring_idx][i].skb = NULL;
 			} else if (rxd->btype == VMXNET3_RXD_BTYPE_BODY &&
 					rq->buf_info[ring_idx][i].page) {
-				pci_unmap_page(adapter->pdev, rxd->addr,
+				dma_unmap_page(&adapter->pdev->dev, rxd->addr,
 					       rxd->len, PCI_DMA_FROMDEVICE);
 				put_page(rq->buf_info[ring_idx][i].page);
 				rq->buf_info[ring_idx][i].page = NULL;
@@ -1400,25 +1409,31 @@ static void vmxnet3_rq_destroy(struct vmxnet3_rx_queue *rq,
 	}
 
 
-	kfree(rq->buf_info[0]);
-
 	for (i = 0; i < 2; i++) {
 		if (rq->rx_ring[i].base) {
-			pci_free_consistent(adapter->pdev, rq->rx_ring[i].size
-					    * sizeof(struct Vmxnet3_RxDesc),
-					    rq->rx_ring[i].base,
-					    rq->rx_ring[i].basePA);
+			dma_free_coherent(&adapter->pdev->dev,
+					  rq->rx_ring[i].size
+					  * sizeof(struct Vmxnet3_RxDesc),
+					  rq->rx_ring[i].base,
+					  rq->rx_ring[i].basePA);
 			rq->rx_ring[i].base = NULL;
 		}
 		rq->buf_info[i] = NULL;
 	}
 
 	if (rq->comp_ring.base) {
-		pci_free_consistent(adapter->pdev, rq->comp_ring.size *
-				    sizeof(struct Vmxnet3_RxCompDesc),
-				    rq->comp_ring.base, rq->comp_ring.basePA);
+		dma_free_coherent(&adapter->pdev->dev, rq->comp_ring.size
+				  * sizeof(struct Vmxnet3_RxCompDesc),
+				  rq->comp_ring.base, rq->comp_ring.basePA);
 		rq->comp_ring.base = NULL;
 	}
+
+	if (rq->buf_info[0]) {
+		size_t sz = sizeof(struct vmxnet3_rx_buf_info) *
+			(rq->rx_ring[0].size + rq->rx_ring[1].size);
+		dma_free_coherent(&adapter->pdev->dev, sz, rq->buf_info[0],
+				  rq->buf_info_pa);
+	}
 }
 
 
@@ -1503,8 +1518,10 @@ vmxnet3_rq_create(struct vmxnet3_rx_queue *rq, struct vmxnet3_adapter *adapter)
 	for (i = 0; i < 2; i++) {
 
 		sz = rq->rx_ring[i].size * sizeof(struct Vmxnet3_RxDesc);
-		rq->rx_ring[i].base = pci_alloc_consistent(adapter->pdev, sz,
-							&rq->rx_ring[i].basePA);
+		rq->rx_ring[i].base = dma_alloc_coherent(
+						&adapter->pdev->dev, sz,
+						&rq->rx_ring[i].basePA,
+						GFP_KERNEL);
 		if (!rq->rx_ring[i].base) {
 			netdev_err(adapter->netdev,
 				   "failed to allocate rx ring %d\n", i);
@@ -1513,8 +1530,9 @@ vmxnet3_rq_create(struct vmxnet3_rx_queue *rq, struct vmxnet3_adapter *adapter)
 	}
 
 	sz = rq->comp_ring.size * sizeof(struct Vmxnet3_RxCompDesc);
-	rq->comp_ring.base = pci_alloc_consistent(adapter->pdev, sz,
-						  &rq->comp_ring.basePA);
+	rq->comp_ring.base = dma_alloc_coherent(&adapter->pdev->dev, sz,
+						&rq->comp_ring.basePA,
+						GFP_KERNEL);
 	if (!rq->comp_ring.base) {
 		netdev_err(adapter->netdev, "failed to allocate rx comp ring\n");
 		goto err;
@@ -1522,7 +1540,8 @@ vmxnet3_rq_create(struct vmxnet3_rx_queue *rq, struct vmxnet3_adapter *adapter)
 
 	sz = sizeof(struct vmxnet3_rx_buf_info) * (rq->rx_ring[0].size +
 						   rq->rx_ring[1].size);
-	bi = kzalloc(sz, GFP_KERNEL);
+	bi = dma_zalloc_coherent(&adapter->pdev->dev, sz, &rq->buf_info_pa,
+				 GFP_KERNEL);
 	if (!bi)
 		goto err;
 
@@ -2005,6 +2024,7 @@ vmxnet3_set_mc(struct net_device *netdev)
 	struct Vmxnet3_RxFilterConf *rxConf =
 					&adapter->shared->devRead.rxFilterConf;
 	u8 *new_table = NULL;
+	dma_addr_t new_table_pa = 0;
 	u32 new_mode = VMXNET3_RXM_UCAST;
 
 	if (netdev->flags & IFF_PROMISC) {
@@ -2028,8 +2048,12 @@ vmxnet3_set_mc(struct net_device *netdev)
 				new_mode |= VMXNET3_RXM_MCAST;
 				rxConf->mfTableLen = cpu_to_le16(
 					netdev_mc_count(netdev) * ETH_ALEN);
-				rxConf->mfTablePA = cpu_to_le64(virt_to_phys(
-						    new_table));
+				new_table_pa = dma_map_single(
+							&adapter->pdev->dev,
+							new_table,
+							rxConf->mfTableLen,
+							PCI_DMA_TODEVICE);
+				rxConf->mfTablePA = cpu_to_le64(new_table_pa);
 			} else {
 				netdev_info(netdev, "failed to copy mcast list"
 					    ", setting ALL_MULTI\n");
@@ -2056,7 +2080,11 @@ vmxnet3_set_mc(struct net_device *netdev)
 			       VMXNET3_CMD_UPDATE_MAC_FILTERS);
 	spin_unlock_irqrestore(&adapter->cmd_lock, flags);
 
-	kfree(new_table);
+	if (new_table) {
+		dma_unmap_single(&adapter->pdev->dev, new_table_pa,
+				 rxConf->mfTableLen, PCI_DMA_TODEVICE);
+		kfree(new_table);
+	}
 }
 
 void
@@ -2096,7 +2124,7 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter *adapter)
 	devRead->misc.driverInfo.vmxnet3RevSpt = cpu_to_le32(1);
 	devRead->misc.driverInfo.uptVerSpt = cpu_to_le32(1);
 
-	devRead->misc.ddPA = cpu_to_le64(virt_to_phys(adapter));
+	devRead->misc.ddPA = cpu_to_le64(adapter->adapter_pa);
 	devRead->misc.ddLen = cpu_to_le32(sizeof(struct vmxnet3_adapter));
 
 	/* set up feature flags */
@@ -2125,7 +2153,7 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter *adapter)
 		tqc->txRingBasePA   = cpu_to_le64(tq->tx_ring.basePA);
 		tqc->dataRingBasePA = cpu_to_le64(tq->data_ring.basePA);
 		tqc->compRingBasePA = cpu_to_le64(tq->comp_ring.basePA);
-		tqc->ddPA           = cpu_to_le64(virt_to_phys(tq->buf_info));
+		tqc->ddPA           = cpu_to_le64(tq->buf_info_pa);
 		tqc->txRingSize     = cpu_to_le32(tq->tx_ring.size);
 		tqc->dataRingSize   = cpu_to_le32(tq->data_ring.size);
 		tqc->compRingSize   = cpu_to_le32(tq->comp_ring.size);
@@ -2143,8 +2171,7 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter *adapter)
 		rqc->rxRingBasePA[0] = cpu_to_le64(rq->rx_ring[0].basePA);
 		rqc->rxRingBasePA[1] = cpu_to_le64(rq->rx_ring[1].basePA);
 		rqc->compRingBasePA  = cpu_to_le64(rq->comp_ring.basePA);
-		rqc->ddPA            = cpu_to_le64(virt_to_phys(
-							rq->buf_info));
+		rqc->ddPA            = cpu_to_le64(rq->buf_info_pa);
 		rqc->rxRingSize[0]   = cpu_to_le32(rq->rx_ring[0].size);
 		rqc->rxRingSize[1]   = cpu_to_le32(rq->rx_ring[1].size);
 		rqc->compRingSize    = cpu_to_le32(rq->comp_ring.size);
@@ -2184,8 +2211,9 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter *adapter)
 				i, adapter->num_rx_queues);
 
 		devRead->rssConfDesc.confVer = 1;
-		devRead->rssConfDesc.confLen = sizeof(*rssConf);
-		devRead->rssConfDesc.confPA  = virt_to_phys(rssConf);
+		devRead->rssConfDesc.confLen = cpu_to_le32(sizeof(*rssConf));
+		devRead->rssConfDesc.confPA =
+			cpu_to_le64(adapter->rss_conf_pa);
 	}
 
 #endif /* VMXNET3_RSS */
@@ -2948,9 +2976,13 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	adapter->pdev = pdev;
 
 	spin_lock_init(&adapter->cmd_lock);
-	adapter->shared = pci_alloc_consistent(adapter->pdev,
-					       sizeof(struct Vmxnet3_DriverShared),
-					       &adapter->shared_pa);
+	adapter->adapter_pa = dma_map_single(&adapter->pdev->dev, adapter,
+					     sizeof(struct vmxnet3_adapter),
+					     PCI_DMA_TODEVICE);
+	adapter->shared = dma_alloc_coherent(
+				&adapter->pdev->dev,
+				sizeof(struct Vmxnet3_DriverShared),
+				&adapter->shared_pa, GFP_KERNEL);
 	if (!adapter->shared) {
 		dev_err(&pdev->dev, "Failed to allocate memory\n");
 		err = -ENOMEM;
@@ -2963,8 +2995,9 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 
 	size = sizeof(struct Vmxnet3_TxQueueDesc) * adapter->num_tx_queues;
 	size += sizeof(struct Vmxnet3_RxQueueDesc) * adapter->num_rx_queues;
-	adapter->tqd_start = pci_alloc_consistent(adapter->pdev, size,
-						  &adapter->queue_desc_pa);
+	adapter->tqd_start = dma_alloc_coherent(&adapter->pdev->dev, size,
+						&adapter->queue_desc_pa,
+						GFP_KERNEL);
 
 	if (!adapter->tqd_start) {
 		dev_err(&pdev->dev, "Failed to allocate memory\n");
@@ -2974,7 +3007,10 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 	adapter->rqd_start = (struct Vmxnet3_RxQueueDesc *)(adapter->tqd_start +
 							    adapter->num_tx_queues);
 
-	adapter->pm_conf = kmalloc(sizeof(struct Vmxnet3_PMConf), GFP_KERNEL);
+	adapter->pm_conf = dma_alloc_coherent(&adapter->pdev->dev,
+					      sizeof(struct Vmxnet3_PMConf),
+					      &adapter->pm_conf_pa,
+					      GFP_KERNEL);
 	if (adapter->pm_conf == NULL) {
 		err = -ENOMEM;
 		goto err_alloc_pm;
@@ -2982,7 +3018,10 @@ vmxnet3_probe_device(struct pci_dev *pdev,
 
 #ifdef VMXNET3_RSS
 
-	adapter->rss_conf = kmalloc(sizeof(struct UPT1_RSSConf), GFP_KERNEL);
+	adapter->rss_conf = dma_alloc_coherent(&adapter->pdev->dev,
+					       sizeof(struct UPT1_RSSConf),
+					       &adapter->rss_conf_pa,
+					       GFP_KERNEL);
 	if (adapter->rss_conf == NULL) {
 		err = -ENOMEM;
 		goto err_alloc_rss;
@@ -3077,17 +3116,22 @@ err_ver:
 	vmxnet3_free_pci_resources(adapter);
 err_alloc_pci:
 #ifdef VMXNET3_RSS
-	kfree(adapter->rss_conf);
+	dma_free_coherent(&adapter->pdev->dev, sizeof(struct UPT1_RSSConf),
+			  adapter->rss_conf, adapter->rss_conf_pa);
 err_alloc_rss:
 #endif
-	kfree(adapter->pm_conf);
+	dma_free_coherent(&adapter->pdev->dev, sizeof(struct Vmxnet3_PMConf),
+			  adapter->pm_conf, adapter->pm_conf_pa);
 err_alloc_pm:
-	pci_free_consistent(adapter->pdev, size, adapter->tqd_start,
-			    adapter->queue_desc_pa);
+	dma_free_coherent(&adapter->pdev->dev, size, adapter->tqd_start,
+			  adapter->queue_desc_pa);
 err_alloc_queue_desc:
-	pci_free_consistent(adapter->pdev, sizeof(struct Vmxnet3_DriverShared),
-			    adapter->shared, adapter->shared_pa);
+	dma_free_coherent(&adapter->pdev->dev,
+			  sizeof(struct Vmxnet3_DriverShared),
+			  adapter->shared, adapter->shared_pa);
 err_alloc_shared:
+	dma_unmap_single(&adapter->pdev->dev, adapter->adapter_pa,
+			 sizeof(struct vmxnet3_adapter), PCI_DMA_TODEVICE);
 	pci_set_drvdata(pdev, NULL);
 	free_netdev(netdev);
 	return err;
@@ -3118,16 +3162,21 @@ vmxnet3_remove_device(struct pci_dev *pdev)
 	vmxnet3_free_intr_resources(adapter);
 	vmxnet3_free_pci_resources(adapter);
 #ifdef VMXNET3_RSS
-	kfree(adapter->rss_conf);
+	dma_free_coherent(&adapter->pdev->dev, sizeof(struct UPT1_RSSConf),
+			  adapter->rss_conf, adapter->rss_conf_pa);
 #endif
-	kfree(adapter->pm_conf);
+	dma_free_coherent(&adapter->pdev->dev, sizeof(struct Vmxnet3_PMConf),
+			  adapter->pm_conf, adapter->pm_conf_pa);
 
 	size = sizeof(struct Vmxnet3_TxQueueDesc) * adapter->num_tx_queues;
 	size += sizeof(struct Vmxnet3_RxQueueDesc) * num_rx_queues;
-	pci_free_consistent(adapter->pdev, size, adapter->tqd_start,
-			    adapter->queue_desc_pa);
-	pci_free_consistent(adapter->pdev, sizeof(struct Vmxnet3_DriverShared),
-			    adapter->shared, adapter->shared_pa);
+	dma_free_coherent(&adapter->pdev->dev, size, adapter->tqd_start,
+			  adapter->queue_desc_pa);
+	dma_free_coherent(&adapter->pdev->dev,
+			  sizeof(struct Vmxnet3_DriverShared),
+			  adapter->shared, adapter->shared_pa);
+	dma_unmap_single(&adapter->pdev->dev, adapter->adapter_pa,
+			 sizeof(struct vmxnet3_adapter), PCI_DMA_TODEVICE);
 	free_netdev(netdev);
 }
 
@@ -3227,8 +3276,8 @@ skip_arp:
 	adapter->shared->devRead.pmConfDesc.confVer = cpu_to_le32(1);
 	adapter->shared->devRead.pmConfDesc.confLen = cpu_to_le32(sizeof(
 								  *pmConf));
-	adapter->shared->devRead.pmConfDesc.confPA = cpu_to_le64(virt_to_phys(
-								 pmConf));
+	adapter->shared->devRead.pmConfDesc.confPA =
+		cpu_to_le64(adapter->pm_conf_pa);
 
 	spin_lock_irqsave(&adapter->cmd_lock, flags);
 	VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD,
@@ -3265,8 +3314,8 @@ vmxnet3_resume(struct device *device)
 	adapter->shared->devRead.pmConfDesc.confVer = cpu_to_le32(1);
 	adapter->shared->devRead.pmConfDesc.confLen = cpu_to_le32(sizeof(
 								  *pmConf));
-	adapter->shared->devRead.pmConfDesc.confPA = cpu_to_le64(virt_to_phys(
-								 pmConf));
+	adapter->shared->devRead.pmConfDesc.confPA =
+		cpu_to_le64(adapter->pm_conf_pa);
 
 	netif_device_attach(netdev);
 	pci_set_power_state(pdev, PCI_D0);
diff --git a/drivers/net/vmxnet3/vmxnet3_int.h b/drivers/net/vmxnet3/vmxnet3_int.h
index 3541814..a03f358 100644
--- a/drivers/net/vmxnet3/vmxnet3_int.h
+++ b/drivers/net/vmxnet3/vmxnet3_int.h
@@ -70,10 +70,10 @@
 /*
  * Version numbers
  */
-#define VMXNET3_DRIVER_VERSION_STRING   "1.1.30.0-k"
+#define VMXNET3_DRIVER_VERSION_STRING   "1.2.0.0-k"
 
 /* a 32-bit int, each byte encode a verion number in VMXNET3_DRIVER_VERSION */
-#define VMXNET3_DRIVER_VERSION_NUM      0x01011E00
+#define VMXNET3_DRIVER_VERSION_NUM      0x01020000
 
 #if defined(CONFIG_PCI_MSI)
 	/* RSS only makes sense if MSI-X is supported. */
@@ -229,6 +229,7 @@ struct vmxnet3_tx_queue {
 	spinlock_t                      tx_lock;
 	struct vmxnet3_cmd_ring         tx_ring;
 	struct vmxnet3_tx_buf_info      *buf_info;
+	dma_addr_t                       buf_info_pa;
 	struct vmxnet3_tx_data_ring     data_ring;
 	struct vmxnet3_comp_ring        comp_ring;
 	struct Vmxnet3_TxQueueCtrl      *shared;
@@ -277,6 +278,7 @@ struct vmxnet3_rx_queue {
 	u32 qid;            /* rqID in RCD for buffer from 1st ring */
 	u32 qid2;           /* rqID in RCD for buffer from 2nd ring */
 	struct vmxnet3_rx_buf_info     *buf_info[2];
+	dma_addr_t                      buf_info_pa;
 	struct Vmxnet3_RxQueueCtrl            *shared;
 	struct vmxnet3_rq_driver_stats  stats;
 } __attribute__((__aligned__(SMP_CACHE_BYTES)));
@@ -353,6 +355,10 @@ struct vmxnet3_adapter {
 	unsigned long  state;    /* VMXNET3_STATE_BIT_xxx */
 
 	int share_intr;
+
+	dma_addr_t adapter_pa;
+	dma_addr_t pm_conf_pa;
+	dma_addr_t rss_conf_pa;
 };
 
 #define VMXNET3_WRITE_BAR0_REG(adapter, reg, val)  \
-- 
1.7.4.1

^ permalink raw reply related

* Re: [PATCH v2 4/5] net: ethernet: cpsw: add support for hardware interface mode config
From: Mugunthan V N @ 2013-08-23 16:38 UTC (permalink / raw)
  To: Daniel Mack
  Cc: netdev, nsekhar, sergei.shtylyov, davem, ujhelyi.m, vaibhav.bedia,
	d-gerlach, linux-arm-kernel, linux-omap, devicetree
In-Reply-To: <1377247417-27386-5-git-send-email-zonque@gmail.com>

On Friday 23 August 2013 02:13 PM, Daniel Mack wrote:
> The cpsw currently lacks code to properly set up the hardware interface
> mode on AM33xx. Other platforms might be equally affected.
>
> Usually, the bootloader will configure the control module register, so
> probably that's why such support wasn't needed in the past. In suspend
> mode though, this register is modified, and so it needs reprogramming
> after resume.
>
> This patch adds code that makes use of the previously added and optional
> support for passing the control mode register, and configures the
> correct register bits when the slave is opened.
>
> The AM33xx also has a bit for each slave to configure the RMII reference
> clock direction. Setting it is now supported by a per-slave DT property.
>
> This code path introducted by this patch is currently exclusive for
> am33xx.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/cpsw.txt |  2 +
>  drivers/net/ethernet/ti/cpsw.c                 | 61 ++++++++++++++++++++++++++
>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>  3 files changed, 64 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index b717458..0895a51 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -34,6 +34,8 @@ Required properties:
>  - phy_id		: Specifies slave phy id
>  - phy-mode		: The interface between the SoC and the PHY (a string
>  			  that of_get_phy_mode() can understand)
> +- ti,rmii-clock-ext	: If present, the driver will configure the RMII
> +			  interface to external clock usage
>  - mac-address		: Specifies slave MAC address
>  
>  Optional properties:
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 2e19de0..6e36f49 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -138,6 +138,13 @@ do {								\
>  #define CPSW_CMINTMAX_INTVL	(1000 / CPSW_CMINTMIN_CNT)
>  #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
>  
> +#define AM33XX_GMII_SEL_MODE_MII	(0)
> +#define AM33XX_GMII_SEL_MODE_RMII	(1)
> +#define AM33XX_GMII_SEL_MODE_RGMII	(2)
> +
> +#define AM33XX_GMII_SEL_RMII2_IO_CLK_EN	BIT(7)
> +#define AM33XX_GMII_SEL_RMII1_IO_CLK_EN	BIT(6)

These defined can go into cpsw.h so that we can maintain platform
defines in one file.

> +
>  #define cpsw_enable_irq(priv)	\
>  	do {			\
>  		u32 i;		\
> @@ -980,6 +987,56 @@ static inline void cpsw_add_dual_emac_def_ale_entries(
>  		priv->host_port, ALE_VLAN, slave->port_vlan);
>  }
>  
> +static void cpsw_set_phy_interface_mode(struct cpsw_slave *slave,
> +					struct cpsw_priv *priv)
> +{
> +	u32 reg, mask, mode = 0;
> +

Please define each variable in separate line to make it clean.
Please check for IS_ERR(gmii_sel_reg) and return if gmii register is not
specified in DT then the below code will crash when trying to access
gmii_sel_reg.

> +	switch (priv->data.hw_type) {
> +	case CPSW_TYPE_AM33XX:
> +		if (!priv->gmii_sel_reg)
> +			break;
> +
> +		reg = readl(priv->gmii_sel_reg);
> +
> +		if (slave->phy) {
> +			switch (slave->phy->interface) {
> +			case PHY_INTERFACE_MODE_MII:
> +			default:
> +				mode = AM33XX_GMII_SEL_MODE_MII;
> +				break;
> +			case PHY_INTERFACE_MODE_RMII:
> +				mode = AM33XX_GMII_SEL_MODE_RMII;
> +				break;
> +			case PHY_INTERFACE_MODE_RGMII:

You need to take care of other RGMII modes as well, for your info
AM335xevm phy mode is "rgmii-txid"

Regards
Mugunthan V N

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Mugunthan V N @ 2013-08-23 16:45 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: Daniel Mack, netdev, bcousson, nsekhar, sergei.shtylyov, davem,
	ujhelyi.m, vaibhav.bedia, d-gerlach, linux-arm-kernel, linux-omap,
	devicetree
In-Reply-To: <52177052.1030308@ti.com>

On Friday 23 August 2013 07:53 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>> In order to support features that are specific to the AM335x IP, we have
>> to add hardware types and another compatible string.
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 4e5ca54..b717458 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>>  ------------------------------------------------------
>>  
>>  Required properties:
>> -- compatible		: Should be "ti,cpsw"
>> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
>> +			  "ti,am3352-cpsw" for AM3352 SoCs
>>  - reg			: physical base address and size of the cpsw
>>  			  registers map.
>>  			  An optional third memory region can be supplied if
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 7a25ff4..73c44cb6 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -155,6 +155,11 @@ do {								\
>>  		((priv->data.dual_emac) ? priv->emac_port :	\
>>  		priv->data.active_slave)
>>  
>> +enum {
>> +	CPSW_TYPE_GENERIC,
>> +	CPSW_TYPE_AM33XX
>> +};
>> +
>>  static int debug_level;
>>  module_param(debug_level, int, 0);
>>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
>> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>>  	slave->port_vlan = data->dual_emac_res_vlan;
>>  }
>>  
>> +static const struct of_device_id cpsw_of_mtable[] = {
>> +	{
>> +		.compatible	= "ti,am3352-cpsw",
> I didn't notice this earlier, but can't you use the IP version
> as a compatible instead of using a SOC name. Whats really SOC specific
> on this IP ? Sorry i have missed any earlier discussion on this but
> this approach doesn't seem good. Its like adding SOC checks in the
> driver subsystem.
>
>
But the same IP can be used in different SoC as well where the control
register may be different as per the Silicon Integration team's decision?

Ideally there should be a separate control module driver so that it can
take care of different SoC related needs.

Regards
Mugunthan V N

^ permalink raw reply

* Re: [PATCH v4 4/5] net: ethernet: cpsw: add support for hardware interface mode config
From: Mugunthan V N @ 2013-08-23 16:50 UTC (permalink / raw)
  To: Daniel Mack
  Cc: netdev, bcousson, nsekhar, sergei.shtylyov, davem, ujhelyi.m,
	vaibhav.bedia, d-gerlach, linux-arm-kernel, linux-omap,
	devicetree
In-Reply-To: <1377267365-24057-5-git-send-email-zonque@gmail.com>

On Friday 23 August 2013 07:46 PM, Daniel Mack wrote:
> The cpsw currently lacks code to properly set up the hardware interface
> mode on AM33xx. Other platforms might be equally affected.
>
> Usually, the bootloader will configure the control module register, so
> probably that's why such support wasn't needed in the past. In suspend
> mode though, this register is modified, and so it needs reprogramming
> after resume.
>
> This patch adds code that makes use of the previously added and optional
> support for passing the control mode register, and configures the
> correct register bits when the slave is opened.
>
> The AM33xx also has a bit for each slave to configure the RMII reference
> clock direction. Setting it is now supported by a per-slave DT property.
>
> This code path introducted by this patch is currently exclusive for
> am33xx.
>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/cpsw.txt |  2 +
>  drivers/net/ethernet/ti/cpsw.c                 | 61 ++++++++++++++++++++++++++
>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>  3 files changed, 64 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index b717458..0895a51 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -34,6 +34,8 @@ Required properties:
>  - phy_id		: Specifies slave phy id
>  - phy-mode		: The interface between the SoC and the PHY (a string
>  			  that of_get_phy_mode() can understand)
> +- ti,rmii-clock-ext	: If present, the driver will configure the RMII
> +			  interface to external clock usage
>  - mac-address		: Specifies slave MAC address
>  
>  Optional properties:
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 73c44cb6..86b8f7a 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -138,6 +138,13 @@ do {								\
>  #define CPSW_CMINTMAX_INTVL	(1000 / CPSW_CMINTMIN_CNT)
>  #define CPSW_CMINTMIN_INTVL	((1000 / CPSW_CMINTMAX_CNT) + 1)
>  
> +#define AM33XX_GMII_SEL_MODE_MII	(0)
> +#define AM33XX_GMII_SEL_MODE_RMII	(1)
> +#define AM33XX_GMII_SEL_MODE_RGMII	(2)
> +
> +#define AM33XX_GMII_SEL_RMII2_IO_CLK_EN	BIT(7)
> +#define AM33XX_GMII_SEL_RMII1_IO_CLK_EN	BIT(6)

These defined can go into cpsw.h so that we can maintain platform
defines in one file.


> +
>  #define cpsw_enable_irq(priv)	\
>  	do {			\
>  		u32 i;		\
> @@ -980,6 +987,56 @@ static inline void cpsw_add_dual_emac_def_ale_entries(
>  		priv->host_port, ALE_VLAN, slave->port_vlan);
>  }
>  
> +static void cpsw_set_phy_interface_mode(struct cpsw_slave *slave,
> +					struct cpsw_priv *priv)
> +{
> +	u32 reg, mask, mode = 0;

Please define each variable in separate line to make it clean.
Please check for IS_ERR(gmii_sel_reg) and return if gmii register is not
specified in DT then the below code will crash when trying to access
gmii_sel_reg.


> +
> +	switch (priv->data.hw_type) {
> +	case CPSW_TYPE_AM33XX:
> +		if (!priv->gmii_sel_reg)
> +			break;
> +
> +		reg = readl(priv->gmii_sel_reg);
> +
> +		if (slave->phy) {
> +			switch (slave->phy->interface) {
> +			case PHY_INTERFACE_MODE_MII:
> +			default:
> +				mode = AM33XX_GMII_SEL_MODE_MII;
> +				break;
> +			case PHY_INTERFACE_MODE_RMII:
> +				mode = AM33XX_GMII_SEL_MODE_RMII;
> +				break;
> +			case PHY_INTERFACE_MODE_RGMII:

You need to take care of other RGMII modes as well, for your info
AM335xevm phy mode is "rgmii-txid"

Regards
Mugunthan V N

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Santosh Shilimkar @ 2013-08-23 16:56 UTC (permalink / raw)
  To: Daniel Mack
  Cc: mugunthanvnm, sergei.shtylyov, d-gerlach, netdev, nsekhar,
	vaibhav.bedia, devicetree, bcousson, ujhelyi.m, linux-omap, davem,
	linux-arm-kernel
In-Reply-To: <52178E20.5040404@gmail.com>

On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
> 
>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>> +	{
>>> +		.compatible	= "ti,am3352-cpsw",
>>
>> I didn't notice this earlier, but can't you use the IP version
>> as a compatible instead of using a SOC name. Whats really SOC specific
>> on this IP ? Sorry i have missed any earlier discussion on this but
>> this approach doesn't seem good. Its like adding SOC checks in the
>> driver subsystem.
> 
> As I already mentioned in the cover letter and in the commit message, I
> just don't know which criteria makes most sense here.
> 
> On a general note, I would say that chances that this exactly IP core
> with the same version number will appear on some other silicon which
> doesn't support the control mode register in an AM33xx fashion, is not
> necessarily negligible.
> 
> So what that new compatible string denotes is the cpsw in a version as
> found on am3352 SoCs, which is actually exactly what it does.
> 
> I don't have a strong opinion here, but see your point. I just don't
> have a better idea on how to treat that.
> 
So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
That way if in future if someone uses those features, they can use
this compatible if they don't they use the one which suites that
SOC.

Regards,
Santosh

^ permalink raw reply

* RE: [net-next v2 1/8] i40e: main driver core
From: Nelson, Shannon @ 2013-08-23 17:00 UTC (permalink / raw)
  To: David Miller, Kirsher, Jeffrey T
  Cc: Brandeburg, Jesse, netdev@vger.kernel.org, gospo@redhat.com,
	sassmann@redhat.com, Waskiewicz Jr, Peter P,
	e1000-devel@lists.sourceforge.net
In-Reply-To: <20130823.002805.1975660490102085087.davem@davemloft.net>

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Friday, August 23, 2013 12:28 AM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com; Nelson, Shannon; Waskiewicz Jr, Peter P; e1000-
> devel@lists.sourceforge.net
> Subject: Re: [net-next v2 1/8] i40e: main driver core

Thanks, Dave, for your time and the notes.

> 
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Thu, 22 Aug 2013 19:15:35 -0700
> 
> > +enum i40e_status_code i40e_allocate_dma_mem_d(struct i40e_hw *hw,
> > +					      struct i40e_dma_mem *mem,
> > +					      u64 size, u32 alignment)
> > +{
>  ...
> > +	mem->va = dma_alloc_coherent(&pf->pdev->dev, mem->size,
> > +				     &mem->pa, GFP_ATOMIC | __GFP_ZERO);
> 
> First, I see no reason to specify GFP_ATOMIC here, code paths that
> call this thing even have comments above them like:
> 
> --------------------
> + *  Do *NOT* hold the lock when calling this as the memory allocation
> routines
> + *  called are not going to be atomic context safe
> --------------------
> 
> Secondly, use dma_zalloc_coherent() if you want __GFP_ZERO.

Sure, we'll adjust.

> 
> > +static int i40e_get_lump(struct i40e_lump_tracking *pile, u16 needed,
> u16 id)
> > +{
> > +	int i = 0, j = 0;
> > +	int ret = I40E_ERR_NO_MEMORY;
> > +
> > +	if (pile == NULL || needed == 0 || id >= I40E_PILE_VALID_BIT) {
> > +		pr_info("%s: param err: pile=%p needed=%d id=0x%04x\n",
> > +		       __func__, pile, needed, id);
> > +		return I40E_ERR_PARAM;
> 
> Since there is absolutely no context passed into these helper routines,
> the log messages are less useful than they could be.  If you did this
> right you could use netdev_info() or dev_info() here.

Perhaps we went a little too far in trying for loosely coupled code?  We'll add enough context for dev_info().

> 
> > +void i40e_pf_reset_stats(struct i40e_pf *pf)
> > +{
> > +	memset(&pf->stats, 0, sizeof(struct i40e_hw_port_stats));
> > +	memset(&pf->stats_offsets, 0, sizeof(struct i40e_hw_port_stats));
> > +	pf->stat_offsets_loaded = false;
> > +
> > +}
> 
> Spurious empty line at end of that function.

Obviously we need another pass at catching these little whitespace issues.

> 
> > +		flush(hw);
> 
> I think this brief and common name is asking for namespace collision
> problems.  Maybe name it i40e_flush or i40e_hw_flush or something like
> that.

Good call - thanks.

> 
> > +{
> > +	int i;
> > +	struct i40e_pf *pf = vsi->back;
> 
> Please order local variable declarations from longest line to shortest.

Will do, on these and the rest.

[...]

> 
> > +static u8 i40e_dcb_get_num_tc(struct i40e_dcbx_config *dcbcfg)
> > +{
> > +	int num_tc = 0, i;
> > +	/* Scan the ETS Config Priority Table to find
> > +	 * traffic class enabled for a given priority
> > +	 * and use the traffic class index to get the
> > +	 * number of traffic classes enabled
> > +	 */
> 
> Please put an empty line between the local variables and
> the rest of the function.

Yep.

[...]

> 
> > +static inline int i40e_prev_power_of_2(int n)
> > +{
> > +	int p = n;
> > +	--p;
> > +	p |= p >> 1;
> > +	p |= p >> 2;
> > +	p |= p >> 4;
> > +	p |= p >> 8;
> > +	p |= p >> 16;
> > +	if (p == (n - 1))
> > +		return n;  /* it was already a power of 2 */
> > +	p >>= 1;
> > +	return ++p;
> > +}
> 
> I think something using rounddown_pow_of_two() would accomplish this.
> 
> Perhaps:
> 
> 	if (!is_power_of_2(x))
> 		x = rounddown_pow_of_two(x);

Oh, didn't know about that one, thanks!

sln

^ permalink raw reply

* Re: [PATCH 1/2] genl: Fix genl dumpit() locking.
From: Pravin Shelar @ 2013-08-23 17:05 UTC (permalink / raw)
  To: Johannes Berg; +Cc: netdev@vger.kernel.org, Jesse Gross
In-Reply-To: <1377242432.14021.2.camel@jlt4.sipsolutions.net>

On Fri, Aug 23, 2013 at 12:20 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> @@ -572,15 +594,29 @@ static int genl_family_rcv_msg(struct
>> genl_family *family,
>>                 return -EPERM;
>>
>>         if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
>> -               struct netlink_dump_control c = {
>> -                       .dump = ops->dumpit,
>> -                       .done = ops->done,
>> -               };
>> +               struct netlink_dump_control c;
>> +               int rc;
>>
>>                 if (ops->dumpit == NULL)
>>                         return -EOPNOTSUPP;
>>
>> -               return netlink_dump_start(net->genl_sock, skb, nlh,
>> &c);
>> +               memset(&c, 0, sizeof(c));
>> +               if (!family->parallel_ops) {
>> +                       genl_unlock();
>> +                       c.data = ops;
>> +                       c.dump = genl_lock_dumpit;
>> +                       if (ops->done)
>> +                               c.done = genl_lock_done;
>> +               } else {
>> +                       c.dump = ops->dumpit;
>> +                       c.done = ops->done;
>> +               }
>> +
>> +               rc = netlink_dump_start(net->genl_sock, skb, nlh, &c);
>> +               if (!family->parallel_ops)
>> +                       genl_lock();
>> +               return rc;
>> +
>
> I think this piece would be easier to read if you call
> netlink_dump_start() separately in the two branches. If you also move
> the "c" variable into the branches then you can keep initializing it
> with an explicit initializer which would also be more readable IMHO.
>
ok, I will update patch.

> Either way, this seems fine. I still think that not assigning cb_mutex
> wasn't the smartest idea, but I'll reply over in the other thread.

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Santosh Shilimkar @ 2013-08-23 17:05 UTC (permalink / raw)
  To: Mugunthan V N
  Cc: Daniel Mack, netdev, bcousson, nsekhar, sergei.shtylyov, davem,
	ujhelyi.m, vaibhav.bedia, d-gerlach, linux-arm-kernel, linux-omap,
	devicetree
In-Reply-To: <521791C1.2060803@ti.com>

On Friday 23 August 2013 12:45 PM, Mugunthan V N wrote:
> On Friday 23 August 2013 07:53 PM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>> In order to support features that are specific to the AM335x IP, we have
>>> to add hardware types and another compatible string.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>>>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>>>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>>> index 4e5ca54..b717458 100644
>>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>>>  ------------------------------------------------------
>>>  
>>>  Required properties:
>>> -- compatible		: Should be "ti,cpsw"
>>> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
>>> +			  "ti,am3352-cpsw" for AM3352 SoCs
>>>  - reg			: physical base address and size of the cpsw
>>>  			  registers map.
>>>  			  An optional third memory region can be supplied if
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>>> index 7a25ff4..73c44cb6 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -155,6 +155,11 @@ do {								\
>>>  		((priv->data.dual_emac) ? priv->emac_port :	\
>>>  		priv->data.active_slave)
>>>  
>>> +enum {
>>> +	CPSW_TYPE_GENERIC,
>>> +	CPSW_TYPE_AM33XX
>>> +};
>>> +
>>>  static int debug_level;
>>>  module_param(debug_level, int, 0);
>>>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
>>> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>>>  	slave->port_vlan = data->dual_emac_res_vlan;
>>>  }
>>>  
>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>> +	{
>>> +		.compatible	= "ti,am3352-cpsw",
>> I didn't notice this earlier, but can't you use the IP version
>> as a compatible instead of using a SOC name. Whats really SOC specific
>> on this IP ? Sorry i have missed any earlier discussion on this but
>> this approach doesn't seem good. Its like adding SOC checks in the
>> driver subsystem.
>>
>>
> But the same IP can be used in different SoC as well where the control
> register may be different as per the Silicon Integration team's decision?
> 
> Ideally there should be a separate control module driver so that it can
> take care of different SoC related needs.
> 
Instead of having one control module driver to address all the requirements
in various IP's, you could specifically address that in subsystem driver layer.
SOC guys invariably use control modules for certain functionality since it
is easy and quick fix. And their is uniformity in the way they are
implemented for different IP needs.

In this specific case, if the CPSW IP version has some part of the
functionality implemented using control module that is just harfaware
choice. Its not mandatory to partition that in two sub-system. Rather
consider it as a special IP version and treat it accordingly in the
driver.

Regards,
Santosh



^ permalink raw reply

* Re: [PATCH 2/2] genl: Hold reference on correct module while netlink-dump.
From: Pravin Shelar @ 2013-08-23 17:05 UTC (permalink / raw)
  To: Johannes Berg; +Cc: netdev@vger.kernel.org, Jesse Gross
In-Reply-To: <1377242625.14021.5.camel@jlt4.sipsolutions.net>

On Fri, Aug 23, 2013 at 12:23 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> +extern int __genl_register_family(struct genl_family *family, struct module *module);
>
> Why add an extra argument instead of just assigning family->module in
> the inline wrapper(s):
>
> static inline int genl_register_family(struct genl_family *family)
> {
>         family->module = THIS_MODULE;
>         return __genl_register_family(family);
> }
>
This is better, I will send updated patch.

^ permalink raw reply

* Re: [net-next v2 3/8] i40e: driver ethtool core
From: Stefan Assmann @ 2013-08-23 17:08 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: davem, Jesse Brandeburg, netdev, gospo, Shannon Nelson,
	PJ Waskiewicz, e1000-devel
In-Reply-To: <1377224142-25160-4-git-send-email-jeffrey.t.kirsher@intel.com>

On 23.08.2013 04:15, Jeff Kirsher wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> This patch contains the ethtool interface and implementation.
> 
> The goal in this patch series is minimal functionality while not
> including much in the way of "set support."

[...]

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c

[...]

> +#define I40E_QUEUE_STATS_LEN(n) \
> +  ((((struct i40e_netdev_priv *)netdev_priv((n)))->vsi->num_queue_pairs + \
> +    ((struct i40e_netdev_priv *)netdev_priv((n)))->vsi->num_queue_pairs) * 2)
> +#define I40E_GLOBAL_STATS_LEN	ARRAY_SIZE(i40e_gstrings_stats)
> +#define I40E_NETDEV_STATS_LEN   ARRAY_SIZE(i40e_gstrings_net_stats)
> +#define I40E_VSI_STATS_LEN(n)   (I40E_NETDEV_STATS_LEN + \

Please use tabs for spacing here.

> +				 I40E_QUEUE_STATS_LEN((n)))
> +#define I40E_PFC_STATS_LEN ( \
> +		(FIELD_SIZEOF(struct i40e_pf, stats.priority_xoff_rx) + \
> +		 FIELD_SIZEOF(struct i40e_pf, stats.priority_xon_rx) + \
> +		 FIELD_SIZEOF(struct i40e_pf, stats.priority_xoff_tx) + \
> +		 FIELD_SIZEOF(struct i40e_pf, stats.priority_xon_tx) + \
> +		 FIELD_SIZEOF(struct i40e_pf, stats.priority_xon_2_xoff)) \
> +		 / sizeof(u64))
> +#define I40E_PF_STATS_LEN(n)    (I40E_GLOBAL_STATS_LEN + \

Here as well.

[...]

> +static void i40e_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
> +			  void *p)
> +{
> +	struct i40e_netdev_priv *np = netdev_priv(netdev);
> +	struct i40e_pf *pf = np->vsi->back;
> +	struct i40e_hw *hw = &pf->hw;
> +	u32 *reg_buf = p;
> +	int i, j, ri;
> +	u32 reg;
> +
> +	/* Tell ethtool which driver-version-specific regs output we have.
> +	 *
> +	 * At some point, if we have ethtool doing special formatting of
> +	 * this data, it will rely on this version number to know how to
> +	 * interpret things.  Hence, this needs to be updated if/when the
> +	 * diags register table is changed.
> +	 */
> +	regs->version = 1;
> +
> +	/* loop through the diags reg table for what to print */
> +	ri = 0;
> +	for (i = 0; i40e_reg_list[i].offset != 0; i++) {
> +		for (j = 0; j < i40e_reg_list[i].elements; j++) {
> +			reg = i40e_reg_list[i].offset
> +				+ (j * i40e_reg_list[i].stride);
> +			reg_buf[ri++] = rd32(hw, reg);
> +		}
> +	}
> +
> +	return;

void function, no return necessary.

> +}

[...]

> +static void i40e_get_ethtool_stats(struct net_device *netdev,
> +				   struct ethtool_stats *stats, u64 *data)
> +{
> +	struct i40e_netdev_priv *np = netdev_priv(netdev);
> +	struct i40e_vsi *vsi = np->vsi;
> +	struct i40e_pf *pf = vsi->back;
> +	struct rtnl_link_stats64 *net_stats = i40e_get_vsi_stats_struct(vsi);
> +	char *p;
> +	int i, j;
> +
> +	i40e_update_stats(vsi);
> +
> +	i = 0;

This could be avoided by int i = 0 few lines above.

> +	for (j = 0; j < I40E_NETDEV_STATS_LEN; j++) {
> +		p = (char *)net_stats + i40e_gstrings_net_stats[j].stat_offset;
> +		data[i++] = (i40e_gstrings_net_stats[j].sizeof_stat ==
> +			sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
> +	}
> +	for (j = 0; j < vsi->num_queue_pairs; j++) {
> +		data[i++] = vsi->tx_rings[j].tx_stats.packets;
> +		data[i++] = vsi->tx_rings[j].tx_stats.bytes;
> +	}
> +	for (j = 0; j < vsi->num_queue_pairs; j++) {
> +		data[i++] = vsi->rx_rings[j].rx_stats.packets;
> +		data[i++] = vsi->rx_rings[j].rx_stats.bytes;
> +	}
> +	if (vsi == pf->vsi[pf->lan_vsi]) {
> +		for (j = 0; j < I40E_GLOBAL_STATS_LEN; j++) {
> +			p = (char *)pf + i40e_gstrings_stats[j].stat_offset;
> +			data[i++] = (i40e_gstrings_stats[j].sizeof_stat ==
> +				   sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
> +		}
> +		for (j = 0; j < I40E_MAX_USER_PRIORITY; j++) {
> +			data[i++] = pf->stats.priority_xon_tx[j];
> +			data[i++] = pf->stats.priority_xoff_tx[j];
> +		}
> +		for (j = 0; j < I40E_MAX_USER_PRIORITY; j++) {
> +			data[i++] = pf->stats.priority_xon_rx[j];
> +			data[i++] = pf->stats.priority_xoff_rx[j];
> +		}
> +		for (j = 0; j < I40E_MAX_USER_PRIORITY; j++)
> +			data[i++] = pf->stats.priority_xon_2_xoff[j];
> +	}
> +
> +	return;

Another void function.

[...]

> +static struct ethtool_ops i40e_ethtool_ops = {
> +	.get_settings           = i40e_get_settings,
> +	.get_drvinfo            = i40e_get_drvinfo,
> +	.get_regs_len           = i40e_get_regs_len,
> +	.get_regs               = i40e_get_regs,
> +	.nway_reset             = i40e_nway_reset,
> +	.get_link               = ethtool_op_get_link,
> +	.get_wol                = i40e_get_wol,
> +	.get_ringparam          = i40e_get_ringparam,
> +	.set_ringparam          = i40e_set_ringparam,
> +	.get_pauseparam         = i40e_get_pauseparam,
> +	.get_msglevel           = i40e_get_msglevel,
> +	.set_msglevel           = i40e_set_msglevel,
> +	.get_rxnfc              = i40e_get_rxnfc,
> +	.set_rxnfc              = i40e_set_rxnfc,
> +	.self_test              = i40e_diag_test,
> +	.get_strings            = i40e_get_strings,
> +	.set_phys_id            = i40e_set_phys_id,
> +	.get_sset_count         = i40e_get_sset_count,
> +	.get_ethtool_stats      = i40e_get_ethtool_stats,
> +	.get_coalesce           = i40e_get_coalesce,
> +	.set_coalesce           = i40e_set_coalesce,
> +	.get_ts_info            = i40e_get_ts_info,
> +};

It would be nice if you could use tabs for spacing here.

  Stefan

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Sekhar Nori @ 2013-08-23 17:09 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: Daniel Mack, netdev, bcousson, sergei.shtylyov, davem, ujhelyi.m,
	mugunthanvnm, vaibhav.bedia, d-gerlach, linux-arm-kernel,
	linux-omap, devicetree
In-Reply-To: <52179448.1020906@ti.com>



On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
>> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>
>>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>>> +	{
>>>> +		.compatible	= "ti,am3352-cpsw",
>>>
>>> I didn't notice this earlier, but can't you use the IP version
>>> as a compatible instead of using a SOC name. Whats really SOC specific
>>> on this IP ? Sorry i have missed any earlier discussion on this but
>>> this approach doesn't seem good. Its like adding SOC checks in the
>>> driver subsystem.
>>
>> As I already mentioned in the cover letter and in the commit message, I
>> just don't know which criteria makes most sense here.
>>
>> On a general note, I would say that chances that this exactly IP core
>> with the same version number will appear on some other silicon which
>> doesn't support the control mode register in an AM33xx fashion, is not
>> necessarily negligible.
>>
>> So what that new compatible string denotes is the cpsw in a version as
>> found on am3352 SoCs, which is actually exactly what it does.
>>
>> I don't have a strong opinion here, but see your point. I just don't
>> have a better idea on how to treat that.
>>
> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.

If this could be handled using IP version then the right way would be to
just read the IP version from hardware and use it. No need of DT property.

Thanks,
Sekhar

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Daniel Mack @ 2013-08-23 17:17 UTC (permalink / raw)
  To: Sekhar Nori
  Cc: Santosh Shilimkar, netdev, bcousson, sergei.shtylyov, davem,
	ujhelyi.m, mugunthanvnm, vaibhav.bedia, d-gerlach,
	linux-arm-kernel, linux-omap, devicetree
In-Reply-To: <5217974B.5090503@ti.com>

On 23.08.2013 19:09, Sekhar Nori wrote:
> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:

>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
> 
> If this could be handled using IP version then the right way would be to
> just read the IP version from hardware and use it. No need of DT property.

Yes, exactly. The information we need to support the phy interface
setting is completely disconnected from the cpsw core itself.


Daniel

^ permalink raw reply

* Deal of $25,275,000.00
From: Q @ 2013-08-23 13:58 UTC (permalink / raw)
  To: Recipients

Deal of $25,275,000.00

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Santosh Shilimkar @ 2013-08-23 17:19 UTC (permalink / raw)
  To: Sekhar Nori
  Cc: mugunthanvnm, sergei.shtylyov, d-gerlach, netdev, vaibhav.bedia,
	Daniel Mack, devicetree, bcousson, ujhelyi.m, linux-omap, davem,
	linux-arm-kernel
In-Reply-To: <5217974B.5090503@ti.com>

On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
> 
> 
> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
>>> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>>>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>>
>>>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>>>> +	{
>>>>> +		.compatible	= "ti,am3352-cpsw",
>>>>
>>>> I didn't notice this earlier, but can't you use the IP version
>>>> as a compatible instead of using a SOC name. Whats really SOC specific
>>>> on this IP ? Sorry i have missed any earlier discussion on this but
>>>> this approach doesn't seem good. Its like adding SOC checks in the
>>>> driver subsystem.
>>>
>>> As I already mentioned in the cover letter and in the commit message, I
>>> just don't know which criteria makes most sense here.
>>>
>>> On a general note, I would say that chances that this exactly IP core
>>> with the same version number will appear on some other silicon which
>>> doesn't support the control mode register in an AM33xx fashion, is not
>>> necessarily negligible.
>>>
>>> So what that new compatible string denotes is the cpsw in a version as
>>> found on am3352 SoCs, which is actually exactly what it does.
>>>
>>> I don't have a strong opinion here, but see your point. I just don't
>>> have a better idea on how to treat that.
>>>
>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
> 
> If this could be handled using IP version then the right way would be to
> just read the IP version from hardware and use it. No need of DT property.
> 
Thats fine as well but I thought the patch needed additional properties like
CM reg-address come from DT and hence the separate compatible. If you can
manage without that, thats even better.

Regards,
Santosh

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Mugunthan V N @ 2013-08-23 17:23 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: Daniel Mack, netdev, bcousson, nsekhar, sergei.shtylyov, davem,
	ujhelyi.m, vaibhav.bedia, d-gerlach, linux-arm-kernel, linux-omap,
	devicetree, Gupta, Pekon
In-Reply-To: <52179448.1020906@ti.com>

On Friday 23 August 2013 10:26 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 12:30 PM, Daniel Mack wrote:
>> On 23.08.2013 16:23, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>>> +	{
>>>> +		.compatible	= "ti,am3352-cpsw",
>>> I didn't notice this earlier, but can't you use the IP version
>>> as a compatible instead of using a SOC name. Whats really SOC specific
>>> on this IP ? Sorry i have missed any earlier discussion on this but
>>> this approach doesn't seem good. Its like adding SOC checks in the
>>> driver subsystem.
>> As I already mentioned in the cover letter and in the commit message, I
>> just don't know which criteria makes most sense here.
>>
>> On a general note, I would say that chances that this exactly IP core
>> with the same version number will appear on some other silicon which
>> doesn't support the control mode register in an AM33xx fashion, is not
>> necessarily negligible.
>>
>> So what that new compatible string denotes is the cpsw in a version as
>> found on am3352 SoCs, which is actually exactly what it does.
>>
>> I don't have a strong opinion here, but see your point. I just don't
>> have a better idea on how to treat that.
>>
> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
> That way if in future if someone uses those features, they can use
> this compatible if they don't they use the one which suites that
> SOC.
>
We cannot map control module register with CPSW IP version as both comes
from different design team and CPSW ip version can be same across SoC
and gmii sel register definition can be different. Control module
defines may vary in different SoC as per SoC requirements.

Adding Pekon Gupta who had worked in Silicon team before.

Regards
Mugunthan V N

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Daniel Mack @ 2013-08-23 17:24 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: Sekhar Nori, netdev, bcousson, sergei.shtylyov, davem, ujhelyi.m,
	mugunthanvnm, vaibhav.bedia, d-gerlach, linux-arm-kernel,
	linux-omap, devicetree
In-Reply-To: <521799A3.7080706@ti.com>

On 23.08.2013 19:19, Santosh Shilimkar wrote:
> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:

>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>
>> If this could be handled using IP version then the right way would be to
>> just read the IP version from hardware and use it. No need of DT property.
>>
> Thats fine as well but I thought the patch needed additional properties like
> CM reg-address come from DT and hence the separate compatible. If you can
> manage without that, thats even better.

We can't, that's the whole point :)

Well, theoretically, we could for now, but that's not a clean solution.
Again: the problem here is that the control port is separated from the
cpsw core, and so we have to implement something specific for the AM3352
SoC. I know that's a violation of clean and generic driver ideas, but
there's no way we can assume that every cpsw v2 ip block has a control
port that is compatible to the one found on am335x chips.


Daniel

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Santosh Shilimkar @ 2013-08-23 17:28 UTC (permalink / raw)
  To: Daniel Mack
  Cc: Sekhar Nori, netdev, bcousson, sergei.shtylyov, davem, ujhelyi.m,
	mugunthanvnm, vaibhav.bedia, d-gerlach, linux-arm-kernel,
	linux-omap, devicetree
In-Reply-To: <52179AD6.8090406@gmail.com>

On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
> 
>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>
>>> If this could be handled using IP version then the right way would be to
>>> just read the IP version from hardware and use it. No need of DT property.
>>>
>> Thats fine as well but I thought the patch needed additional properties like
>> CM reg-address come from DT and hence the separate compatible. If you can
>> manage without that, thats even better.
> 
> We can't, that's the whole point :)
> 
I saw that from the patch :)

> Well, theoretically, we could for now, but that's not a clean solution.
> Again: the problem here is that the control port is separated from the
> cpsw core, and so we have to implement something specific for the AM3352
> SoC. I know that's a violation of clean and generic driver ideas, but
> there's no way we can assume that every cpsw v2 ip block has a control
> port that is compatible to the one found on am335x chips.
> 
But there is a possibility that other SOC will just use the same
control module approach. So using a revision IP is just fine. BTW,
CPSW is not the only driver where control module is used. There are
many examples like USB, MMC etc

There is nothing wrong in using the version info and associating
the additional functionality with it.

Regards,
Santosh

^ permalink raw reply

* Re: [PATCH v4 2/5] net: ethernet: cpsw: add optional third memory region for CONTROL module
From: Sergei Shtylyov @ 2013-08-23 17:37 UTC (permalink / raw)
  To: Daniel Mack
  Cc: netdev, bcousson, nsekhar, davem, ujhelyi.m, mugunthanvnm,
	vaibhav.bedia, d-gerlach, linux-arm-kernel, linux-omap,
	devicetree
In-Reply-To: <52178BED.1070308@gmail.com>

On 08/23/2013 08:21 PM, Daniel Mack wrote:

>>> +	priv->gmii_sel_reg = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(priv->gmii_sel_reg)) {
>>> +		dev_err(priv->dev, "unable to map control i/o region\n");

>>      You didn't actually seem to heed my words about error message.

> Well yes I did, but only in the check for platform_get_resource(). As
> the comment says - we pass on if that memory region is not given, but if
> it is given, it also has to be valid.

    Yes, but what I told you was devm_ioremap_resource() prints the error 
messages itself, so that you don't have to. And you even consented with that. :-)

>> And don't  you want to do:

>> 	res = PTR_ERR(priv->gmii_sel_reg);

     Well, I've messed with the variable name: 'res' is struct resource *', 
what I meant was *int* variable.

> Erm, of course. Sorry for that.

> Daniel

WBR, Sergei

^ permalink raw reply

* Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string
From: Sekhar Nori @ 2013-08-23 17:39 UTC (permalink / raw)
  To: Santosh Shilimkar
  Cc: Daniel Mack, netdev, bcousson, sergei.shtylyov, davem, ujhelyi.m,
	mugunthanvnm, vaibhav.bedia, d-gerlach, linux-arm-kernel,
	linux-omap, devicetree
In-Reply-To: <52179BDA.50603@ti.com>

On 8/23/2013 10:58 PM, Santosh Shilimkar wrote:
> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote:
>> On 23.08.2013 19:19, Santosh Shilimkar wrote:
>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote:
>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote:
>>
>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc.
>>>>
>>>> If this could be handled using IP version then the right way would be to
>>>> just read the IP version from hardware and use it. No need of DT property.
>>>>
>>> Thats fine as well but I thought the patch needed additional properties like
>>> CM reg-address come from DT and hence the separate compatible. If you can
>>> manage without that, thats even better.
>>
>> We can't, that's the whole point :)
>>
> I saw that from the patch :)
> 
>> Well, theoretically, we could for now, but that's not a clean solution.
>> Again: the problem here is that the control port is separated from the
>> cpsw core, and so we have to implement something specific for the AM3352
>> SoC. I know that's a violation of clean and generic driver ideas, but
>> there's no way we can assume that every cpsw v2 ip block has a control
>> port that is compatible to the one found on am335x chips.
>>
> But there is a possibility that other SOC will just use the same
> control module approach. So using a revision IP is just fine. BTW,

But this is misleading because it makes appear like the same compatible
can be used on on another SoC like DRA7 which probably has the same
version of IP but a different control module implementation, when in
practice it cannot.

The fact is we are doing something SoC specific in the driver and we
cannot hide that behind IP versions. If really in practice there comes
another SoC with the same control module definition then it can always
use ti,am3352-cpsw compatible as well. The compatible name does not
preclude its usage.

Thanks,
Sekhar

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox