Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1398571 > unrolled thread

Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-05-11 01:20 +0200
    Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Florian Fainelli <f.fainelli@gmail.com> - 2016-05-11 01:30 +0200
      Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-05-11 04:30 +0200
        Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller  driver Timur Tabi <timur@codeaurora.org> - 2016-05-11 22:30 +0200

#1398571 — Re: [PATCH 1/2] [v4] net: emac: emac gigabit ethernet controller driver

Florian Fainelli wrote:
> Are you utilizing the PHYLIB APIs properly? You need at least a
> phy_start() to start the PHY state machine, and an adjust_link callback
> to be provided to phy_connect() (or of_phy_connect()) to manage link
> state changes. And that's the very basic minimum here, there could be
> additional APIs that you may end up using.
>
> There are tons of example in tree of drivers doing this, bcmgenet,
> bcmsysport, tg3 etc.

Thank you.  I think I finally got phylib working, more or less.

Unfortunately, it seems I have some kind of race condition.  The driver 
has a lot that's wrong with it, and I'm trying to fix it all.  One crazy 
the driver does is it create a workqueue to handle a lot of the tasks 
that would normally be handled in the interrupt handler itself.

With phylib support, I know my driver can call phy_mac_interrupt() when 
it gets a link status change interrupt.  I then have an .adjust_link 
callback which starts or stops the mac accordingly.

My problem is that I'm not really sure what adjust_link is supposed to 
be doing.  In addition, it seems that I need to keep the workqueue 
running, otherwise the interface will not function.  I bring the 
interface up, and the driver reports success, but pings do not work.

I'm getting really frustrated.  The sample code isn't really helping a 
whole lot, because I lack a fundamental understanding of what needs to 
be done.  None of the documentation I've read is helpful, and I don't 
know how to debug it.

Can you give me some advice on how to debug this?

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [next] | [standalone]

#1398575

On 05/10/2016 04:18 PM, Timur Tabi wrote:
> Florian Fainelli wrote:
>> Are you utilizing the PHYLIB APIs properly? You need at least a
>> phy_start() to start the PHY state machine, and an adjust_link callback
>> to be provided to phy_connect() (or of_phy_connect()) to manage link
>> state changes. And that's the very basic minimum here, there could be
>> additional APIs that you may end up using.
>>
>> There are tons of example in tree of drivers doing this, bcmgenet,
>> bcmsysport, tg3 etc.
> 
> Thank you.  I think I finally got phylib working, more or less.
> 
> Unfortunately, it seems I have some kind of race condition.  The driver
> has a lot that's wrong with it, and I'm trying to fix it all.  One crazy
> the driver does is it create a workqueue to handle a lot of the tasks
> that would normally be handled in the interrupt handler itself.

That sounds like a typicall top half/bottom half split, fair enough.

> 
> With phylib support, I know my driver can call phy_mac_interrupt() when
> it gets a link status change interrupt.  I then have an .adjust_link
> callback which starts or stops the mac accordingly.

The Ethernet MAC should be started in ndo_open() and stopped in
ndo_close(), in between, there are link state changes, but you are not
supposed to stop or start your Ethernet MAC and its DMA for instance
during link change, if that is a HW requirement, your HW is pretty funky.

> 
> My problem is that I'm not really sure what adjust_link is supposed to
> be doing.

Well, it's pretty simple, it is about re-configuring your Ethernet MAC
based on what the PHY link state mandates: duplex, pause, speed changes,
EEE etc is what this callback is supposed to take care of, at the
Ethernet MAC level.

>  In addition, it seems that I need to keep the workqueue
> running, otherwise the interface will not function.  I bring the
> interface up, and the driver reports success, but pings do not work.
> 
> I'm getting really frustrated.  The sample code isn't really helping a
> whole lot, because I lack a fundamental understanding of what needs to
> be done.  None of the documentation I've read is helpful, and I don't
> know how to debug it.

Seriously, no documentation is helpful? The PHY library seems pretty
well documented to me, but I suppose I have a bias, oh, and patches are
welcome of course.

> 
> Can you give me some advice on how to debug this?

Take a look at drivers/net/ethernet/broadcom/genet/bcmgenet.c and see
how it deals with managing link state changes for instance. The code is
pretty straight forward: link interrupt (and other causes) trigger a
workqueue schedule, which then processes link state changes and calls
phy_mac_interrupt(), which in turn makes the PHY library adjust the
interface carrier state.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1398642

Florian Fainelli wrote:
> The Ethernet MAC should be started in ndo_open() and stopped in
> ndo_close(), in between, there are link state changes, but you are not
> supposed to stop or start your Ethernet MAC and its DMA for instance
> during link change, if that is a HW requirement, your HW is pretty funky.

I think the problem is that the current driver seems to be too eager to 
start/stop the MAC.

Please take a look at emac_work_thread_link_check() at 
https://lkml.org/lkml/2016/4/13/670.  Every time the PHY link goes up, 
it does this:

if (phy->link_up) {
	if (netif_carrier_ok(netdev))
		goto link_task_done;

	pm_runtime_get_sync(netdev->dev.parent);
	netif_info(adpt, timer, adpt->netdev, "NIC Link is Up %s\n",
		   speed);

	emac_mac_start(adpt);
	netif_carrier_on(netdev);
	netif_wake_queue(netdev);


The call to emac_mac_start seems wrong to me here.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[toc] | [prev] | [next] | [standalone]

#1399491

Timur Tabi wrote:
> I think the problem is that the current driver seems to be too eager to
> start/stop the MAC.
>
> Please take a look at emac_work_thread_link_check() at
> https://lkml.org/lkml/2016/4/13/670.  Every time the PHY link goes up,
> it does this:

Never mind, I figured out the problem.  I still have a lot of work ahead 
of me, but at least I'm not stuck any more.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web