| Message ID | 1389166847-3780-4-git-send-email-aaron.f.brown@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote: > From: Mark Rustad <mark.d.rustad@intel.com> > > -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg))) > +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value) Bummer, now you have a all-caps func name. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jan 8, 2014, at 12:47 AM, Scott Feldman <sfeldma@cumulusnetworks.com> wrote: > > On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote: > >> From: Mark Rustad <mark.d.rustad@intel.com> >> >> -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg))) >> +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value) > > Bummer, now you have a all-caps func name. Agreed, but this is actually a fairly common condition among drivers that used to use macros. It isn't perfect, but at least it is moving in the right direction. I'd rather leave the case change for a later patch series that does only that or has some reason to touch all of the register access sites. At least the new accessor I introduced is lower case. :-)
From: "Rustad, Mark D" <mark.d.rustad@intel.com> Date: Thu, 9 Jan 2014 17:34:18 +0000 > On Jan 8, 2014, at 12:47 AM, Scott Feldman <sfeldma@cumulusnetworks.com> wrote: > >> >> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote: >> >>> From: Mark Rustad <mark.d.rustad@intel.com> >>> >>> -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg))) >>> +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value) >> >> Bummer, now you have a all-caps func name. > > Agreed, but this is actually a fairly common condition among drivers that used to use macros. It isn't perfect, but at least it is moving in the right direction. I'd rather leave the case change for a later patch series that does only that or has some reason to touch all of the register access sites. > > At least the new accessor I introduced is lower case. :-) Please address this feedback, all caps function names are really not appropriate. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jan 9, 2014, at 11:39 AM, David Miller <davem@davemloft.net> wrote: > From: "Rustad, Mark D" <mark.d.rustad@intel.com> > Date: Thu, 9 Jan 2014 17:34:18 +0000 > >> On Jan 8, 2014, at 12:47 AM, Scott Feldman <sfeldma@cumulusnetworks.com> wrote: >> >>> >>> On Jan 7, 2014, at 11:40 PM, Aaron Brown <aaron.f.brown@intel.com> wrote: >>> >>>> From: Mark Rustad <mark.d.rustad@intel.com> >>>> >>>> -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg))) >>>> +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value) >>> >>> Bummer, now you have a all-caps func name. >> >> Agreed, but this is actually a fairly common condition among drivers that used to use macros. It isn't perfect, but at least it is moving in the right direction. I'd rather leave the case change for a later patch series that does only that or has some reason to touch all of the register access sites. >> >> At least the new accessor I introduced is lower case. :-) > > Please address this feedback, all caps function names are really not > appropriate. I really don't think it is a good idea to do that as part of this patch series. It makes this patch series a pretty solid barrier to any other patches going into this driver because it would change the name of all the register accessors. This makes me want to deal with that as a separate issue, since there would be no functional reason to drop such a patch and it can be planned into a workflow. Obviously I could do it here, but I *really* think it is procedurally a really bad idea to change the case as part of a functional change. I thought I was doing a favor my at least making them inlines, but prehaps not. Anyone want to take on changing the upper case static inlines in mcf8390, 7990, benet, ns83820, s2io, vxge, iwlwifi, ath9k, wil6210, mwifiex, and rtlwifi? And those are just under drivers/net.
From: "Rustad, Mark D" <mark.d.rustad@intel.com> Date: Thu, 9 Jan 2014 20:14:51 +0000 > Obviously I could do it here, but I *really* think it is > procedurally a really bad idea to change the case as part of a > functional change. I thought I was doing a favor my at least making > them inlines, but prehaps not. It is never a good idea to allow functions to have all caps names and vice versa. Please don't use "difficulty" as a reason to violate this. Doing things right is sometimes hard, I'm sorry to inform you :) > Anyone want to take on changing the upper case static inlines in > mcf8390, 7990, benet, ns83820, s2io, vxge, iwlwifi, ath9k, wil6210, > mwifiex, and rtlwifi? And those are just under drivers/net. Sorry the "crap exists elsewhere, therefore I can make crap too" argument never holds any water. Just because crap exists elsewhere, doesn't mean you should duplicate it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jan 9, 2014, at 12:19 PM, David Miller <davem@davemloft.net> wrote: > From: "Rustad, Mark D" <mark.d.rustad@intel.com> > Date: Thu, 9 Jan 2014 20:14:51 +0000 I'm sorry you dropped my entire procedural argument for not combining the changing of the case with the implementation of the static inlines. That really was and is important! I'm not opposed to changing the case. My concern is for procedurally when and how it gets done. >> Obviously I could do it here, but I *really* think it is >> procedurally a really bad idea to change the case as part of a >> functional change. I thought I was doing a favor my at least making >> them inlines, but prehaps not. > > It is never a good idea to allow functions to have all caps > names and vice versa. Please don't use "difficulty" as a reason > to violate this. > > Doing things right is sometimes hard, I'm sorry to inform you :) It is just that sometimes things take multiple steps. This was just one step. I'm sorry that I may have not made that clear enough. Doing the work is absolutely not the problem, managing the process so everyone can continue working is a concern. >> Anyone want to take on changing the upper case static inlines in >> mcf8390, 7990, benet, ns83820, s2io, vxge, iwlwifi, ath9k, wil6210, >> mwifiex, and rtlwifi? And those are just under drivers/net. > > Sorry the "crap exists elsewhere, therefore I can make crap too" > argument never holds any water. > > Just because crap exists elsewhere, doesn't mean you should duplicate it. I'm sorry that the presence misled me into thinking that it was acceptable at least in a transitional phase. I think I have found a way to address the barrier issue. I will add upper case macros that call the lower case inlines, so for a time both forms may exist, and then have a patch that will update the call sites, and then a patch to remove the macros. Is that acceptable? If only one person were working in this area and everything were serial, it would be much easier to manage this kind of change. When that is not the case, it is better for things to happen in separate steps.
From: "Rustad, Mark D" <mark.d.rustad@intel.com> Date: Thu, 9 Jan 2014 20:46:33 +0000 > I think I have found a way to address the barrier issue. I will add > upper case macros that call the lower case inlines, so for a time > both forms may exist, and then have a patch that will update the > call sites, and then a patch to remove the macros. > > Is that acceptable? Yes. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h index 8da263a..199cf74 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h @@ -585,6 +585,11 @@ static inline u16 ixgbe_desc_unused(struct ixgbe_ring *ring) return ((ntc > ntu) ? 0 : ring->count) + ntc - ntu - 1; } +static inline void ixgbe_write_tail(struct ixgbe_ring *ring, u32 value) +{ + writel(value, ring->tail); +} + #define IXGBE_RX_DESC(R, i) \ (&(((union ixgbe_adv_rx_desc *)((R)->desc))[i])) #define IXGBE_TX_DESC(R, i) \ diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h index d259dc7..5e157ac 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h @@ -124,22 +124,34 @@ s32 ixgbe_reset_pipeline_82599(struct ixgbe_hw *hw); s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw); s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw); -#define IXGBE_WRITE_REG(a, reg, value) writel((value), ((a)->hw_addr + (reg))) +static inline void IXGBE_WRITE_REG(struct ixgbe_hw *hw, u32 reg, u32 value) +{ + writel(value, hw->hw_addr + reg); +} #ifndef writeq -#define writeq(val, addr) writel((u32) (val), addr); \ - writel((u32) (val >> 32), (addr + 4)); +static inline void writeq(u64 val, void __iomem *addr) +{ + writel((u32)val, addr); + writel((u32)(val >> 32), addr + 4); +} #endif -#define IXGBE_WRITE_REG64(a, reg, value) writeq((value), ((a)->hw_addr + (reg))) +static inline void IXGBE_WRITE_REG64(struct ixgbe_hw *hw, u32 reg, u64 value) +{ + writeq(value, hw->hw_addr + reg); +} -#define IXGBE_READ_REG(a, reg) readl((a)->hw_addr + (reg)) +static inline u32 IXGBE_READ_REG(struct ixgbe_hw *hw, u32 reg) +{ + return readl(hw->hw_addr + reg); +} -#define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) (\ - writel((value), ((a)->hw_addr + (reg) + ((offset) << 2)))) +#define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \ + IXGBE_WRITE_REG((a), (reg) + ((offset) << 2), (value)) -#define IXGBE_READ_REG_ARRAY(a, reg, offset) (\ - readl((a)->hw_addr + (reg) + ((offset) << 2))) +#define IXGBE_READ_REG_ARRAY(a, reg, offset) \ + IXGBE_READ_REG((a), (reg) + ((offset) << 2)) #define IXGBE_WRITE_FLUSH(a) IXGBE_READ_REG(a, IXGBE_STATUS) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 923b0fa..4d71277 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -1315,7 +1315,7 @@ static inline void ixgbe_release_rx_desc(struct ixgbe_ring *rx_ring, u32 val) * such as IA-64). */ wmb(); - writel(val, rx_ring->tail); + ixgbe_write_tail(rx_ring, val); } static bool ixgbe_alloc_mapped_page(struct ixgbe_ring *rx_ring, @@ -6699,7 +6699,7 @@ static void ixgbe_tx_map(struct ixgbe_ring *tx_ring, tx_ring->next_to_use = i; /* notify HW of packet */ - writel(i, tx_ring->tail); + ixgbe_write_tail(tx_ring, i); return; dma_error: