[PATCH v2 2/4] ath9k_hw: distinguish single-chip solutions on initial probe print

[PATCH v2 2/4] ath9k_hw: distinguish single-chip solutions on initial probe print

[Luis R. Rodriguez]

Devices with external radios have revisions which we can count on.
On single chip solutions these EEPROM values for these radio revision
also exist but are not meaningful as the radios are embedded onto the
same chip. Each single-chip device evolves together as one device.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---

Changes hw_name to 60 bytes.

 drivers/net/wireless/ath/ath9k/ahb.c |   11 ++++-------
 drivers/net/wireless/ath/ath9k/hw.c  |   27 +++++++++++++++++++++++----
 drivers/net/wireless/ath/ath9k/hw.h  |    3 +--
 drivers/net/wireless/ath/ath9k/pci.c |   11 ++++-------
 4 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
index 41422c4..33da2e7 100644
--- a/drivers/net/wireless/ath/ath9k/ahb.c
+++ b/drivers/net/wireless/ath/ath9k/ahb.c
@@ -69,6 +69,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
 	int irq;
 	int ret = 0;
 	struct ath_hw *ah;
+	char hw_name[60];
 
 	if (!pdev->dev.platform_data) {
 		dev_err(&pdev->dev, "no platform data specified\n");
@@ -133,15 +134,11 @@ static int ath_ahb_probe(struct platform_device *pdev)
 	}
 
 	ah = sc->sc_ah;
+	ath9k_hw_name(ah, hw_name);
 	printk(KERN_INFO
-	       "%s: Atheros AR%s MAC/BB Rev:%x, "
-	       "AR%s RF Rev:%x, mem=0x%lx, irq=%d\n",
+	       "%s: %s mem=0x%lx, irq=%d\n",
 	       wiphy_name(hw->wiphy),
-	       ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
-	       ah->hw_version.macRev,
-	       ath9k_hw_rf_name((ah->hw_version.analog5GhzRev &
-				 AR_RADIO_SREV_MAJOR)),
-	       ah->hw_version.phyRev,
+	       hw_name,
 	       (unsigned long)mem, irq);
 
 	return 0;
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index cf543e3..f65b591 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -3959,7 +3959,7 @@ static struct {
 /*
  * Return the MAC/BB name. "????" is returned if the MAC/BB is unknown.
  */
-const char *ath9k_hw_mac_bb_name(u32 mac_bb_version)
+static const char *ath9k_hw_mac_bb_name(u32 mac_bb_version)
 {
 	int i;
 
@@ -3971,13 +3971,12 @@ const char *ath9k_hw_mac_bb_name(u32 mac_bb_version)
 
 	return "????";
 }
-EXPORT_SYMBOL(ath9k_hw_mac_bb_name);
 
 /*
  * Return the RF name. "????" is returned if the RF is unknown.
  * Used for devices with external radios.
  */
-const char *ath9k_hw_rf_name(u16 rf_version)
+static const char *ath9k_hw_rf_name(u16 rf_version)
 {
 	int i;
 
@@ -3989,4 +3988,24 @@ const char *ath9k_hw_rf_name(u16 rf_version)
 
 	return "????";
 }
-EXPORT_SYMBOL(ath9k_hw_rf_name);
+
+void ath9k_hw_name(struct ath_hw *ah, char *hw_name)
+{
+	/* chipsets >= AR9280 are single-chip */
+	if (AR_SREV_9280_10_OR_LATER(ah)) {
+		sprintf(hw_name,
+			"Atheros single-chip AR%s Rev:%x",
+			ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
+			ah->hw_version.macRev);
+	}
+	else {
+		sprintf(hw_name,
+			"Atheros AR%s MAC/BB Rev:%x AR%s RF Rev:%x: ",
+			ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
+			ah->hw_version.macRev,
+			ath9k_hw_rf_name((ah->hw_version.analog5GhzRev &
+					 AR_RADIO_SREV_MAJOR)),
+			ah->hw_version.phyRev);
+	}
+}
+EXPORT_SYMBOL(ath9k_hw_name);
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 100fcba..3ceb4c6 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -720,8 +720,7 @@ void ath_gen_timer_free(struct ath_hw *ah, struct ath_gen_timer *timer);
 void ath_gen_timer_isr(struct ath_hw *hw);
 u32 ath9k_hw_gettsf32(struct ath_hw *ah);
 
-const char *ath9k_hw_mac_bb_name(u32 mac_bb_version);
-const char *ath9k_hw_rf_name(u16 rf_version);
+void ath9k_hw_name(struct ath_hw *ah, char *hw_name);
 
 #define ATH_PCIE_CAP_LINK_CTRL	0x70
 #define ATH_PCIE_CAP_LINK_L0S	1
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 76f3890..961a18b 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -114,6 +114,7 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	u32 val;
 	int ret = 0;
 	struct ath_hw *ah;
+	char hw_name[60];
 
 	if (pci_enable_device(pdev))
 		return -EIO;
@@ -218,15 +219,11 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	sc->irq = pdev->irq;
 
 	ah = sc->sc_ah;
+	ath9k_hw_name(ah, hw_name);
 	printk(KERN_INFO
-	       "%s: Atheros AR%s MAC/BB Rev:%x "
-	       "AR%s RF Rev:%x: mem=0x%lx, irq=%d\n",
+	       "%s: %s mem=0x%lx, irq=%d\n",
 	       wiphy_name(hw->wiphy),
-	       ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
-	       ah->hw_version.macRev,
-	       ath9k_hw_rf_name((ah->hw_version.analog5GhzRev &
-				 AR_RADIO_SREV_MAJOR)),
-	       ah->hw_version.phyRev,
+	       hw_name,
 	       (unsigned long)mem, pdev->irq);
 
 	return 0;
-- 
1.6.0.4

Re: [PATCH v2 2/4] ath9k_hw: distinguish single-chip solutions on initial probe print

[Kalle Valo]

"Luis R. Rodriguez" <lrodriguez@atheros.com> writes:

> Devices with external radios have revisions which we can count on.
> On single chip solutions these EEPROM values for these radio revision
> also exist but are not meaningful as the radios are embedded onto the
> same chip. Each single-chip device evolves together as one device.
>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>
> Changes hw_name to 60 bytes.

Thanks, much better now.

Another minor comment, sorry that I didn't notice it earlier:

> +void ath9k_hw_name(struct ath_hw *ah, char *hw_name)
> +{
> +	/* chipsets >= AR9280 are single-chip */
> +	if (AR_SREV_9280_10_OR_LATER(ah)) {
> +		sprintf(hw_name,
> +			"Atheros single-chip AR%s Rev:%x",
> +			ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
> +			ah->hw_version.macRev);
> +	}
> +	else {
> +		sprintf(hw_name,
> +			"Atheros AR%s MAC/BB Rev:%x AR%s RF Rev:%x: ",
> +			ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
> +			ah->hw_version.macRev,
> +			ath9k_hw_rf_name((ah->hw_version.analog5GhzRev &
> +					 AR_RADIO_SREV_MAJOR)),
> +			ah->hw_version.phyRev);
> +	}
> +}

I think you should provide the size of hw_name to this function and
use snprintf() to avoid writing out of bounds.

-- 
Kalle Valo

Re: [PATCH v2 2/4] ath9k_hw: distinguish single-chip solutions on initial probe print

[Luis R. Rodriguez]

On Mon, Oct 26, 2009 at 01:31:44PM -0700, Kalle Valo wrote:
> "Luis R. Rodriguez" <lrodriguez@atheros.com> writes:
> 
> > Devices with external radios have revisions which we can count on.
> > On single chip solutions these EEPROM values for these radio revision
> > also exist but are not meaningful as the radios are embedded onto the
> > same chip. Each single-chip device evolves together as one device.
> >
> > Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> > ---
> >
> > Changes hw_name to 60 bytes.
> 
> Thanks, much better now.
> 
> Another minor comment, sorry that I didn't notice it earlier:
> 
> > +void ath9k_hw_name(struct ath_hw *ah, char *hw_name)
> > +{
> > +     /* chipsets >= AR9280 are single-chip */
> > +     if (AR_SREV_9280_10_OR_LATER(ah)) {
> > +             sprintf(hw_name,
> > +                     "Atheros single-chip AR%s Rev:%x",
> > +                     ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
> > +                     ah->hw_version.macRev);
> > +     }
> > +     else {
> > +             sprintf(hw_name,
> > +                     "Atheros AR%s MAC/BB Rev:%x AR%s RF Rev:%x: ",
> > +                     ath9k_hw_mac_bb_name(ah->hw_version.macVersion),
> > +                     ah->hw_version.macRev,
> > +                     ath9k_hw_rf_name((ah->hw_version.analog5GhzRev &
> > +                                      AR_RADIO_SREV_MAJOR)),
> > +                     ah->hw_version.phyRev);
> > +     }
> > +}
> 
> I think you should provide the size of hw_name to this function and
> use snprintf() to avoid writing out of bounds.

I was going to do this but since I know the users of it and control
it seemed to not matter. I'll respin with this added, better to be careful.

Thanks for the review.

  Luis

Re: [PATCH v2 2/4] ath9k_hw: distinguish single-chip solutions on initial probe print

[Kalle Valo]

"Luis R. Rodriguez" <lrodriguez@Atheros.com> writes:

>> I think you should provide the size of hw_name to this function and
>> use snprintf() to avoid writing out of bounds.
>
> I was going to do this but since I know the users of it and control
> it seemed to not matter. I'll respin with this added, better to be careful.

Yeah, you never know if someone (for example a crazy Finn just after a
refreshing sauna) comes along, changes the function a bit "to clean it
up" and creates a subtle bug ;)

-- 
Kalle Valo

Re: [PATCH v2 2/4] ath9k_hw: distinguish single-chip solutions on initial probe print

[Luis R. Rodriguez]

On Mon, Oct 26, 2009 at 02:15:13PM -0700, Kalle Valo wrote:
> "Luis R. Rodriguez" <lrodriguez@Atheros.com> writes:
> 
> >> I think you should provide the size of hw_name to this function and
> >> use snprintf() to avoid writing out of bounds.
> >
> > I was going to do this but since I know the users of it and control
> > it seemed to not matter. I'll respin with this added, better to be careful.
> 
> Yeah, you never know if someone (for example a crazy Finn just after a
> refreshing sauna) comes along, changes the function a bit "to clean it
> up" and creates a subtle bug ;)

Ah yes, forgot about the crazy sauna fins, must fix now for sure.

  Luis

Re: [PATCH v2 2/4] ath9k_hw: distinguish single-chip solutions on initial probe print

[Luis R. Rodriguez]

On Mon, Oct 26, 2009 at 2:32 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> On Mon, Oct 26, 2009 at 02:15:13PM -0700, Kalle Valo wrote:
>> "Luis R. Rodriguez" <lrodriguez@Atheros.com> writes:
>>
>> >> I think you should provide the size of hw_name to this function and
>> >> use snprintf() to avoid writing out of bounds.
>> >
>> > I was going to do this but since I know the users of it and control
>> > it seemed to not matter. I'll respin with this added, better to be careful.
>>
>> Yeah, you never know if someone (for example a crazy Finn just after a
>> refreshing sauna) comes along, changes the function a bit "to clean it
>> up" and creates a subtle bug ;)
>
> Ah yes, forgot about the crazy sauna fins, must fix now for sure.

I have a couple of other fixes and since I no longer need to debug
ath9k_hw_reset() I'm going to drop the last patch in this series. I'll
resend this series then with the new patches.

  Luis