Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net,stable 1/1] net: fec: match the dev_id between probe and remove
@ 2019-11-29  6:40 Andy Duan
  2019-11-30 20:27 ` David Miller
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Duan @ 2019-11-29  6:40 UTC (permalink / raw)
  To: davem; +Cc: netdev, Andy Duan

Test device bind/unbind on i.MX8QM platform:
echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/unbind
echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/bind

error log:
pps pps0: new PPS source ptp0 /sys/bus/platform/drivers/fec/bind
fec: probe of 5b040000.ethernet failed with error -2

It should decrease the dev_id when device is unbinded. So let
the fec_dev_id as global variable and let the count match in
.probe() and .remvoe().

Reported-by: shivani.patel <shivani.patel@volansystech.com>
Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 05c1899..348fd8a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -243,6 +243,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 	(addr < txq->tso_hdrs_dma + txq->bd.ring_size * TSO_HEADER_SIZE))
 
 static int mii_cnt;
+static int fec_dev_id;
 
 static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp,
 					     struct bufdesc_prop *bd)
@@ -3397,7 +3398,6 @@ fec_probe(struct platform_device *pdev)
 	struct net_device *ndev;
 	int i, irq, ret = 0;
 	const struct of_device_id *of_id;
-	static int dev_id;
 	struct device_node *np = pdev->dev.of_node, *phy_node;
 	int num_tx_qs;
 	int num_rx_qs;
@@ -3442,7 +3442,7 @@ fec_probe(struct platform_device *pdev)
 	}
 
 	fep->pdev = pdev;
-	fep->dev_id = dev_id++;
+	fep->dev_id = fec_dev_id++;
 
 	platform_set_drvdata(pdev, ndev);
 
@@ -3623,7 +3623,7 @@ fec_probe(struct platform_device *pdev)
 		of_phy_deregister_fixed_link(np);
 	of_node_put(phy_node);
 failed_phy:
-	dev_id--;
+	fec_dev_id--;
 failed_ioremap:
 	free_netdev(ndev);
 
@@ -3653,6 +3653,7 @@ fec_drv_remove(struct platform_device *pdev)
 		of_phy_deregister_fixed_link(np);
 	of_node_put(fep->phy_node);
 	free_netdev(ndev);
+	fec_dev_id--;
 
 	clk_disable_unprepare(fep->clk_ahb);
 	clk_disable_unprepare(fep->clk_ipg);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net,stable 1/1] net: fec: match the dev_id between probe and remove
  2019-11-29  6:40 [PATCH net,stable 1/1] net: fec: match the dev_id between probe and remove Andy Duan
@ 2019-11-30 20:27 ` David Miller
  2019-12-02  3:04   ` [EXT] " Andy Duan
  0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2019-11-30 20:27 UTC (permalink / raw)
  To: fugang.duan; +Cc: netdev

From: Andy Duan <fugang.duan@nxp.com>
Date: Fri, 29 Nov 2019 06:40:28 +0000

> Test device bind/unbind on i.MX8QM platform:
> echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/unbind
> echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/bind
> 
> error log:
> pps pps0: new PPS source ptp0 /sys/bus/platform/drivers/fec/bind
> fec: probe of 5b040000.ethernet failed with error -2
> 
> It should decrease the dev_id when device is unbinded. So let
> the fec_dev_id as global variable and let the count match in
> .probe() and .remvoe().
> 
> Reported-by: shivani.patel <shivani.patel@volansystech.com>
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>

This is not correct.

Nothing says that there is a direct correlation between the devices
added and the ones removed, nor the order in which these operations
occur relative to eachother.

This dev_id allocation is buggy because you aren't using a proper
ID allocation scheme such as IDR.

I'm not applying this patch, it is incorrect and makes things
worse rather than better.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [EXT] Re: [PATCH net,stable 1/1] net: fec: match the dev_id between probe and remove
  2019-11-30 20:27 ` David Miller
@ 2019-12-02  3:04   ` " Andy Duan
  2019-12-02  3:18     ` Florian Fainelli
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Duan @ 2019-12-02  3:04 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> Sent: Sunday, December 1, 2019 4:28 AM
> From: Andy Duan <fugang.duan@nxp.com>
> Date: Fri, 29 Nov 2019 06:40:28 +0000
> 
> > Test device bind/unbind on i.MX8QM platform:
> > echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/unbind
> > echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/bind
> >
> > error log:
> > pps pps0: new PPS source ptp0 /sys/bus/platform/drivers/fec/bind
> > fec: probe of 5b040000.ethernet failed with error -2
> >
> > It should decrease the dev_id when device is unbinded. So let the
> > fec_dev_id as global variable and let the count match in
> > .probe() and .remvoe().
> >
> > Reported-by: shivani.patel <shivani.patel@volansystech.com>
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> 
> This is not correct.
> 
> Nothing says that there is a direct correlation between the devices added and
> the ones removed, nor the order in which these operations occur relative to
> eachother.
> 
> This dev_id allocation is buggy because you aren't using a proper ID allocation
> scheme such as IDR.
David, you are correct. There still has issue to support bind/unbind feature even if use IDR
to allocate ID because enet instance#1 depend on instance#0 internal MDIO bus for some platforms
and we don't know who is the real instance#0 while binging the device.

Do you have any suggestion to implement the bind/unbind feature with current dependence?
Thanks.

Andy
> 
> I'm not applying this patch, it is incorrect and makes things worse rather than
> better.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [EXT] Re: [PATCH net,stable 1/1] net: fec: match the dev_id between probe and remove
  2019-12-02  3:04   ` [EXT] " Andy Duan
@ 2019-12-02  3:18     ` Florian Fainelli
  2019-12-02  3:42       ` Andy Duan
  0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2019-12-02  3:18 UTC (permalink / raw)
  To: Andy Duan, David Miller; +Cc: netdev



On 12/1/2019 7:04 PM, Andy Duan wrote:
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> Sent: Sunday, December 1, 2019 4:28 AM
>> From: Andy Duan <fugang.duan@nxp.com>
>> Date: Fri, 29 Nov 2019 06:40:28 +0000
>>
>>> Test device bind/unbind on i.MX8QM platform:
>>> echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/unbind
>>> echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/bind
>>>
>>> error log:
>>> pps pps0: new PPS source ptp0 /sys/bus/platform/drivers/fec/bind
>>> fec: probe of 5b040000.ethernet failed with error -2
>>>
>>> It should decrease the dev_id when device is unbinded. So let the
>>> fec_dev_id as global variable and let the count match in
>>> .probe() and .remvoe().
>>>
>>> Reported-by: shivani.patel <shivani.patel@volansystech.com>
>>> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
>>
>> This is not correct.
>>
>> Nothing says that there is a direct correlation between the devices added and
>> the ones removed, nor the order in which these operations occur relative to
>> eachother.
>>
>> This dev_id allocation is buggy because you aren't using a proper ID allocation
>> scheme such as IDR.
> David, you are correct. There still has issue to support bind/unbind feature even if use IDR
> to allocate ID because enet instance#1 depend on instance#0 internal MDIO bus for some platforms
> and we don't know who is the real instance#0 while binging the device.
> 
> Do you have any suggestion to implement the bind/unbind feature with current dependence?
> Thanks.

Can you use the device driver model to reflect the link between the MDIO
bus device, its parent Ethernet controller and the second instance
Ethernet controller? Be it through the use of device links, or an actual
dev->parent relationship?
-- 
Florian

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [EXT] Re: [PATCH net,stable 1/1] net: fec: match the dev_id between probe and remove
  2019-12-02  3:18     ` Florian Fainelli
@ 2019-12-02  3:42       ` Andy Duan
  0 siblings, 0 replies; 5+ messages in thread
From: Andy Duan @ 2019-12-02  3:42 UTC (permalink / raw)
  To: Florian Fainelli, David Miller; +Cc: netdev

From: Florian Fainelli <f.fainelli@gmail.com> Sent: Monday, December 2, 2019 11:18 AM
> On 12/1/2019 7:04 PM, Andy Duan wrote:
> > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org>
> > Sent: Sunday, December 1, 2019 4:28 AM
> >> From: Andy Duan <fugang.duan@nxp.com>
> >> Date: Fri, 29 Nov 2019 06:40:28 +0000
> >>
> >>> Test device bind/unbind on i.MX8QM platform:
> >>> echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/unbind
> >>> echo 5b040000.ethernet > /sys/bus/platform/drivers/fec/bind
> >>>
> >>> error log:
> >>> pps pps0: new PPS source ptp0 /sys/bus/platform/drivers/fec/bind
> >>> fec: probe of 5b040000.ethernet failed with error -2
> >>>
> >>> It should decrease the dev_id when device is unbinded. So let the
> >>> fec_dev_id as global variable and let the count match in
> >>> .probe() and .remvoe().
> >>>
> >>> Reported-by: shivani.patel <shivani.patel@volansystech.com>
> >>> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> >>
> >> This is not correct.
> >>
> >> Nothing says that there is a direct correlation between the devices
> >> added and the ones removed, nor the order in which these operations
> >> occur relative to eachother.
> >>
> >> This dev_id allocation is buggy because you aren't using a proper ID
> >> allocation scheme such as IDR.
> > David, you are correct. There still has issue to support bind/unbind
> > feature even if use IDR to allocate ID because enet instance#1 depend
> > on instance#0 internal MDIO bus for some platforms and we don't know
> who is the real instance#0 while binging the device.
> >
> > Do you have any suggestion to implement the bind/unbind feature with
> current dependence?
> > Thanks.
> 
> Can you use the device driver model to reflect the link between the MDIO bus
> device, its parent Ethernet controller and the second instance Ethernet
> controller? Be it through the use of device links, or an actual
> dev->parent relationship?

For device dependence, device links maybe the good solution.
Since there instance#1 only depends on instance#0 internal mdio bus,
maybe hive off mdio bus driver from instance@0 is a better solution.

@Florian Fainelli, Thanks for your suggestion.

Andy
> --
> Florian

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29  6:40 [PATCH net,stable 1/1] net: fec: match the dev_id between probe and remove Andy Duan
2019-11-30 20:27 ` David Miller
2019-12-02  3:04   ` [EXT] " Andy Duan
2019-12-02  3:18     ` Florian Fainelli
2019-12-02  3:42       ` Andy Duan

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git