diff mbox

[3/9] net: ethernet: ti: cpts: rework initialization/deinitialization

Message ID 20160914130231.3035-4-grygorii.strashko@ti.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Grygorii Strashko Sept. 14, 2016, 1:02 p.m. UTC 
The current implementation CPTS initialization and deinitialization
(represented by cpts_register/unregister()) is pretty entangled and
has some issues, like:
- ptp clock registered before spinlock, which is protecting it, and
before timecounter and cyclecounter initialization;
- CPTS ref_clk requested using devm API while cpts_register() is
called from .ndo_open(), as result additional checks required;
- CPTS ref_clk is prepared, but never unprepared;
- CPTS is not disabled even when unregistered..

Hence, make things simpler and fix above issues by adding
cpts_create()/cpts_release() which should be called from
.probe()/.remove() respectively and move all static initialization
there. Clean up and update cpts_register/unregister() so PTP clock is
registered the last and unregistered first. In addition, this change
allows to clean up cpts.h for the case when CPTS is disabled.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c |  24 ++++----
 drivers/net/ethernet/ti/cpts.c | 125 ++++++++++++++++++++++++++---------------
 drivers/net/ethernet/ti/cpts.h |  26 +++++++--
 3 files changed, 113 insertions(+), 62 deletions(-)

Comments

Richard Cochran Sept. 14, 2016, 1:52 p.m. UTC | #1 
On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
> @@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>  	u64 ns;
>  	struct skb_shared_hwtstamps *ssh;
>  
> -	if (!cpts->rx_enable)
> +	if (!cpts || !cpts->rx_enable)
>  		return;

This function is in the hot path, and you have added a pointless new
test.  Don't do that.

>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
>  	if (!ns)
> @@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>  	u64 ns;
>  	struct skb_shared_hwtstamps ssh;
>  
> -	if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
> +	if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>  		return;

Same here.

>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
>  	if (!ns)
> @@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>  	skb_tstamp_tx(skb, &ssh);
>  }
>  
> -int cpts_register(struct device *dev, struct cpts *cpts,
> -		  u32 mult, u32 shift)
> +int cpts_register(struct cpts *cpts)
>  {
>  	int err, i;
> -	unsigned long flags;
>  
> -	cpts->info = cpts_info;
> -	cpts->clock = ptp_clock_register(&cpts->info, dev);
> -	if (IS_ERR(cpts->clock)) {
> -		err = PTR_ERR(cpts->clock);
> -		cpts->clock = NULL;
> -		return err;
> -	}
> -	spin_lock_init(&cpts->lock);
> -
> -	cpts->cc.read = cpts_systim_read;
> -	cpts->cc.mask = CLOCKSOURCE_MASK(32);
> -	cpts->cc_mult = mult;
> -	cpts->cc.mult = mult;
> -	cpts->cc.shift = shift;
> +	if (!cpts)
> +		return -EINVAL;

Not hot path, but still silly.  The caller should never pass NULL.

Thanks,
Richard
Grygorii Strashko Sept. 14, 2016, 8:10 p.m. UTC | #2 
On 09/14/2016 04:52 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
>> @@ -323,7 +307,7 @@ void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>>  	u64 ns;
>>  	struct skb_shared_hwtstamps *ssh;
>>  
>> -	if (!cpts->rx_enable)
>> +	if (!cpts || !cpts->rx_enable)
>>  		return;
> 
> This function is in the hot path, and you have added a pointless new
> test.  Don't do that.
> 
>>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
>>  	if (!ns)
>> @@ -338,7 +322,7 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>>  	u64 ns;
>>  	struct skb_shared_hwtstamps ssh;
>>  
>> -	if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>> +	if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
>>  		return;
> 
> Same here.
> 
>>  	ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
>>  	if (!ns)
>> @@ -348,53 +332,102 @@ void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
>>  	skb_tstamp_tx(skb, &ssh);
>>  }
>>  
>> -int cpts_register(struct device *dev, struct cpts *cpts,
>> -		  u32 mult, u32 shift)
>> +int cpts_register(struct cpts *cpts)
>>  {
>>  	int err, i;
>> -	unsigned long flags;
>>  
>> -	cpts->info = cpts_info;
>> -	cpts->clock = ptp_clock_register(&cpts->info, dev);
>> -	if (IS_ERR(cpts->clock)) {
>> -		err = PTR_ERR(cpts->clock);
>> -		cpts->clock = NULL;
>> -		return err;
>> -	}
>> -	spin_lock_init(&cpts->lock);
>> -
>> -	cpts->cc.read = cpts_systim_read;
>> -	cpts->cc.mask = CLOCKSOURCE_MASK(32);
>> -	cpts->cc_mult = mult;
>> -	cpts->cc.mult = mult;
>> -	cpts->cc.shift = shift;
>> +	if (!cpts)
>> +		return -EINVAL;
> 
> Not hot path, but still silly.  The caller should never pass NULL.

Ok. I can't say I'd like all this checks, but there are internal requirement 
to allow CPTS to be disabled though DT on KS2 (even if built in).
I'd try to clarify and return back here.

But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
Richard Cochran Sept. 14, 2016, 8:32 p.m. UTC | #3 
On Wed, Sep 14, 2016 at 11:10:48PM +0300, Grygorii Strashko wrote:
> On 09/14/2016 04:52 PM, Richard Cochran wrote:
> > On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:

> >> -	if (!cpts->rx_enable)
> >> +	if (!cpts || !cpts->rx_enable)
> >>  		return;

> Ok. I can't say I'd like all this checks, but there are internal requirement 
> to allow CPTS to be disabled though DT on KS2 (even if built in).
> I'd try to clarify and return back here.
> 
> But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?

Look at that code snippet.  We already test for @cpts->rx_enable.  If
you want to disable cpts at run time, just avoid setting that field.
There is no need for any new tests or return codes.

Thanks,
Richard
Grygorii Strashko Sept. 14, 2016, 8:37 p.m. UTC | #4 
On 09/14/2016 11:32 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:10:48PM +0300, Grygorii Strashko wrote:
>> On 09/14/2016 04:52 PM, Richard Cochran wrote:
>>> On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
> 
>>>> -	if (!cpts->rx_enable)
>>>> +	if (!cpts || !cpts->rx_enable)
>>>>  		return;
> 
>> Ok. I can't say I'd like all this checks, but there are internal requirement 
>> to allow CPTS to be disabled though DT on KS2 (even if built in).
>> I'd try to clarify and return back here.
>>
>> But It'll be good to know your position - acceptable/can be discussed/completely unacceptable?
> 
> Look at that code snippet.  We already test for @cpts->rx_enable.  If
> you want to disable cpts at run time, just avoid setting that field.
> There is no need for any new tests or return codes.
> 

The problem is that if cpts not initialized than pinter on 
cpts (in consumer/parent driver - NETCP) will be NULL.
So, rx_enable will be unaccessible and cause crash :(
Richard Cochran Sept. 14, 2016, 8:52 p.m. UTC | #5 
On Wed, Sep 14, 2016 at 11:37:45PM +0300, Grygorii Strashko wrote:
> The problem is that if cpts not initialized than pinter on 
> cpts (in consumer/parent driver - NETCP) will be NULL.

You made that problem with your "clean up" in this series.
Previously, cpts was always allocated.

> So, rx_enable will be unaccessible and cause crash :(

So just make sure cpts is initialized.

Thanks,
Richard
Grygorii Strashko Sept. 14, 2016, 8:59 p.m. UTC | #6 
On 09/14/2016 11:52 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 11:37:45PM +0300, Grygorii Strashko wrote:
>> The problem is that if cpts not initialized than pinter on 
>> cpts (in consumer/parent driver - NETCP) will be NULL.
> 
> You made that problem with your "clean up" in this series.
> Previously, cpts was always allocated.

not exactly - it was allocated in CPSW .probe() manually, 
in without this re-work it has to be allocated in NTCP the
same way - manually. So, checks we are discussing here will be still
present, but they will need to be done in CPSW/NETCP drivers -
just one level up and duplicated in both these drivers.

> 
>> So, rx_enable will be unaccessible and cause crash :(
> 
> So just make sure cpts is initialized.

OK. I'll update code this way.
Richard Cochran Sept. 15, 2016, 8:13 a.m. UTC | #7 
On Wed, Sep 14, 2016 at 04:02:25PM +0300, Grygorii Strashko wrote:
> The current implementation CPTS initialization and deinitialization
> (represented by cpts_register/unregister()) is pretty entangled and
> has some issues, like:
> - ptp clock registered before spinlock, which is protecting it, and
> before timecounter and cyclecounter initialization;
> - CPTS ref_clk requested using devm API while cpts_register() is
> called from .ndo_open(), as result additional checks required;
> - CPTS ref_clk is prepared, but never unprepared;
> - CPTS is not disabled even when unregistered..

This list of four items is a clear sign that this one patch should be
broken into a series of four.

Thanks,
Richard
diff mbox

Patch

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 9b900f0..dfd5707 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1406,9 +1406,7 @@  static int cpsw_ndo_open(struct net_device *ndev)
 		if (ret < 0)
 			goto err_cleanup;
 
-		if (cpts_register(cpsw->dev, cpsw->cpts,
-				  cpsw->data.cpts_clock_mult,
-				  cpsw->data.cpts_clock_shift))
+		if (cpts_register(cpsw->cpts))
 			dev_err(priv->dev, "error registering cpts device\n");
 
 	}
@@ -2551,6 +2549,7 @@  static int cpsw_probe(struct platform_device *pdev)
 	struct cpdma_params		dma_params;
 	struct cpsw_ale_params		ale_params;
 	void __iomem			*ss_regs;
+	void __iomem			*cpts_regs;
 	struct resource			*res, *ss_res;
 	const struct of_device_id	*of_id;
 	struct gpio_descs		*mode;
@@ -2575,12 +2574,6 @@  static int cpsw_probe(struct platform_device *pdev)
 	priv->dev  = &ndev->dev;
 	priv->msg_enable = netif_msg_init(debug_level, CPSW_DEBUG);
 	cpsw->rx_packet_max = max(rx_packet_max, 128);
-	cpsw->cpts = devm_kzalloc(&pdev->dev, sizeof(struct cpts), GFP_KERNEL);
-	if (!cpsw->cpts) {
-		dev_err(&pdev->dev, "error allocating cpts\n");
-		ret = -ENOMEM;
-		goto clean_ndev_ret;
-	}
 
 	mode = devm_gpiod_get_array_optional(&pdev->dev, "mode", GPIOD_OUT_LOW);
 	if (IS_ERR(mode)) {
@@ -2669,7 +2662,7 @@  static int cpsw_probe(struct platform_device *pdev)
 	switch (cpsw->version) {
 	case CPSW_VERSION_1:
 		cpsw->host_port_regs = ss_regs + CPSW1_HOST_PORT_OFFSET;
-		cpsw->cpts->reg      = ss_regs + CPSW1_CPTS_OFFSET;
+		cpts_regs		= ss_regs + CPSW1_CPTS_OFFSET;
 		cpsw->hw_stats	     = ss_regs + CPSW1_HW_STATS;
 		dma_params.dmaregs   = ss_regs + CPSW1_CPDMA_OFFSET;
 		dma_params.txhdp     = ss_regs + CPSW1_STATERAM_OFFSET;
@@ -2683,7 +2676,7 @@  static int cpsw_probe(struct platform_device *pdev)
 	case CPSW_VERSION_3:
 	case CPSW_VERSION_4:
 		cpsw->host_port_regs = ss_regs + CPSW2_HOST_PORT_OFFSET;
-		cpsw->cpts->reg      = ss_regs + CPSW2_CPTS_OFFSET;
+		cpts_regs		= ss_regs + CPSW2_CPTS_OFFSET;
 		cpsw->hw_stats	     = ss_regs + CPSW2_HW_STATS;
 		dma_params.dmaregs   = ss_regs + CPSW2_CPDMA_OFFSET;
 		dma_params.txhdp     = ss_regs + CPSW2_STATERAM_OFFSET;
@@ -2749,6 +2742,14 @@  static int cpsw_probe(struct platform_device *pdev)
 		goto clean_dma_ret;
 	}
 
+	cpsw->cpts = cpts_create(cpsw->dev, cpts_regs,
+				 cpsw->data.cpts_clock_mult,
+				 cpsw->data.cpts_clock_shift);
+	if (IS_ERR(cpsw->cpts)) {
+		ret = PTR_ERR(cpsw->cpts);
+		goto clean_ale_ret;
+	}
+
 	ndev->irq = platform_get_irq(pdev, 1);
 	if (ndev->irq < 0) {
 		dev_err(priv->dev, "error getting irq resource\n");
@@ -2857,6 +2858,7 @@  static int cpsw_remove(struct platform_device *pdev)
 		unregister_netdev(cpsw->slaves[1].ndev);
 	unregister_netdev(ndev);
 
+	cpts_release(cpsw->cpts);
 	cpsw_ale_destroy(cpsw->ale);
 	cpdma_ctlr_destroy(cpsw->dma);
 	of_platform_depopulate(&pdev->dev);
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index aaab08e..a46478e 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -228,22 +228,6 @@  static void cpts_overflow_check(struct work_struct *work)
 	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
 }
 
-static void cpts_clk_init(struct device *dev, struct cpts *cpts)
-{
-	cpts->refclk = devm_clk_get(dev, "cpts");
-	if (IS_ERR(cpts->refclk)) {
-		dev_err(dev, "Failed to get cpts refclk\n");
-		cpts->refclk = NULL;
-		return;
-	}
-	clk_prepare_enable(cpts->refclk);
-}
-
-static void cpts_clk_release(struct cpts *cpts)
-{
-	clk_disable(cpts->refclk);
-}
-
 static int cpts_match(struct sk_buff *skb, unsigned int ptp_class,
 		      u16 ts_seqid, u8 ts_msgtype)
 {
@@ -323,7 +307,7 @@  void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 	u64 ns;
 	struct skb_shared_hwtstamps *ssh;
 
-	if (!cpts->rx_enable)
+	if (!cpts || !cpts->rx_enable)
 		return;
 	ns = cpts_find_ts(cpts, skb, CPTS_EV_RX);
 	if (!ns)
@@ -338,7 +322,7 @@  void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 	u64 ns;
 	struct skb_shared_hwtstamps ssh;
 
-	if (!(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
+	if (!cpts || !(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
 		return;
 	ns = cpts_find_ts(cpts, skb, CPTS_EV_TX);
 	if (!ns)
@@ -348,53 +332,102 @@  void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 	skb_tstamp_tx(skb, &ssh);
 }
 
-int cpts_register(struct device *dev, struct cpts *cpts,
-		  u32 mult, u32 shift)
+int cpts_register(struct cpts *cpts)
 {
 	int err, i;
-	unsigned long flags;
 
-	cpts->info = cpts_info;
-	cpts->clock = ptp_clock_register(&cpts->info, dev);
-	if (IS_ERR(cpts->clock)) {
-		err = PTR_ERR(cpts->clock);
-		cpts->clock = NULL;
-		return err;
-	}
-	spin_lock_init(&cpts->lock);
-
-	cpts->cc.read = cpts_systim_read;
-	cpts->cc.mask = CLOCKSOURCE_MASK(32);
-	cpts->cc_mult = mult;
-	cpts->cc.mult = mult;
-	cpts->cc.shift = shift;
+	if (!cpts)
+		return -EINVAL;
 
 	INIT_LIST_HEAD(&cpts->events);
 	INIT_LIST_HEAD(&cpts->pool);
 	for (i = 0; i < CPTS_MAX_EVENTS; i++)
 		list_add(&cpts->pool_data[i].list, &cpts->pool);
 
-	cpts_clk_init(dev, cpts);
+	clk_enable(cpts->refclk);
+
 	cpts_write32(cpts, CPTS_EN, control);
 	cpts_write32(cpts, TS_PEND_EN, int_enable);
 
-	spin_lock_irqsave(&cpts->lock, flags);
+	cpts->cc.mult = cpts->cc_mult;
 	timecounter_init(&cpts->tc, &cpts->cc, ktime_to_ns(ktime_get_real()));
-	spin_unlock_irqrestore(&cpts->lock, flags);
-
-	INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
-	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
 
+	cpts->info = cpts_info;
+	cpts->clock = ptp_clock_register(&cpts->info, cpts->dev);
+	if (IS_ERR(cpts->clock)) {
+		err = PTR_ERR(cpts->clock);
+		cpts->clock = NULL;
+		goto err_ptp;
+	}
 	cpts->phc_index = ptp_clock_index(cpts->clock);
+
+	schedule_delayed_work(&cpts->overflow_work, CPTS_OVERFLOW_PERIOD);
 	return 0;
+
+err_ptp:
+	clk_disable(cpts->refclk);
+	return err;
 }
 
 void cpts_unregister(struct cpts *cpts)
 {
-	if (cpts->clock) {
-		ptp_clock_unregister(cpts->clock);
-		cancel_delayed_work_sync(&cpts->overflow_work);
+	if (!cpts)
+		return;
+
+	if (WARN_ON(!cpts->clock))
+		return;
+
+	cancel_delayed_work_sync(&cpts->overflow_work);
+
+	ptp_clock_unregister(cpts->clock);
+	cpts->clock = NULL;
+
+	cpts_write32(cpts, 0, int_enable);
+	cpts_write32(cpts, 0, control);
+
+	clk_disable(cpts->refclk);
+}
+
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+			 u32 mult, u32 shift)
+{
+	struct cpts *cpts;
+
+	if (!regs || !dev)
+		return ERR_PTR(-EINVAL);
+
+	cpts = devm_kzalloc(dev, sizeof(*cpts), GFP_KERNEL);
+	if (!cpts)
+		return ERR_PTR(-ENOMEM);
+
+	cpts->dev = dev;
+	cpts->reg = (struct cpsw_cpts __iomem *)regs;
+	spin_lock_init(&cpts->lock);
+	INIT_DELAYED_WORK(&cpts->overflow_work, cpts_overflow_check);
+
+	cpts->refclk = devm_clk_get(dev, "cpts");
+	if (IS_ERR(cpts->refclk)) {
+		dev_err(dev, "Failed to get cpts refclk\n");
+		return ERR_PTR(PTR_ERR(cpts->refclk));
 	}
-	if (cpts->refclk)
-		cpts_clk_release(cpts);
+
+	clk_prepare(cpts->refclk);
+
+	cpts->cc.read = cpts_systim_read;
+	cpts->cc.mask = CLOCKSOURCE_MASK(32);
+	cpts->cc.shift = shift;
+	cpts->cc_mult = mult;
+
+	return cpts;
+}
+
+void cpts_release(struct cpts *cpts)
+{
+	if (!cpts)
+		return;
+
+	if (!cpts->refclk)
+		return;
+
+	clk_unprepare(cpts->refclk);
 }
diff --git a/drivers/net/ethernet/ti/cpts.h b/drivers/net/ethernet/ti/cpts.h
index fec753c..0c02f48 100644
--- a/drivers/net/ethernet/ti/cpts.h
+++ b/drivers/net/ethernet/ti/cpts.h
@@ -20,6 +20,8 @@ 
 #ifndef _TI_CPTS_H_
 #define _TI_CPTS_H_
 
+#ifdef CONFIG_TI_CPTS
+
 #include <linux/clk.h>
 #include <linux/clkdev.h>
 #include <linux/clocksource.h>
@@ -108,10 +110,10 @@  struct cpts_event {
 };
 
 struct cpts {
+	struct device *dev;
 	struct cpsw_cpts __iomem *reg;
 	int tx_enable;
 	int rx_enable;
-#ifdef CONFIG_TI_CPTS
 	struct ptp_clock_info info;
 	struct ptp_clock *clock;
 	spinlock_t lock; /* protects time registers */
@@ -124,14 +126,15 @@  struct cpts {
 	struct list_head events;
 	struct list_head pool;
 	struct cpts_event pool_data[CPTS_MAX_EVENTS];
-#endif
 };
 
-#ifdef CONFIG_TI_CPTS
 void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb);
 void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb);
-int cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift);
+int cpts_register(struct cpts *cpts);
 void cpts_unregister(struct cpts *cpts);
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+			 u32 mult, u32 shift);
+void cpts_release(struct cpts *cpts);
 
 static inline void cpts_rx_enable(struct cpts *cpts, int enable)
 {
@@ -156,6 +159,8 @@  static inline bool cpts_is_tx_enabled(struct cpts *cpts)
 }
 
 #else
+struct cpts;
+
 static inline void cpts_rx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 {
 }
@@ -163,8 +168,19 @@  static inline void cpts_tx_timestamp(struct cpts *cpts, struct sk_buff *skb)
 {
 }
 
+static inline
+struct cpts *cpts_create(struct device *dev, void __iomem *regs,
+			 u32 mult, u32 shift)
+{
+	return NULL;
+}
+
+static inline void cpts_release(struct cpts *cpts)
+{
+}
+
 static inline int
-cpts_register(struct device *dev, struct cpts *cpts, u32 mult, u32 shift)
+cpts_register(struct cpts *cpts)
 {
 	return 0;
 }