| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 01 Dec 2014 13:20:48 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Petri Gynther <pgynther@...gle.com>
CC: netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
Kevin Cernekee <cernekee@...adcom.com>
Subject: Re: [PATCH net-next] net: bcmgenet: enable driver to work without
a device tree
On 01/12/14 13:13, Petri Gynther wrote:
> Hi Florian,
>
> Getting back to this (finally). I have made changes per your comments.
> Sending a new patch shortly.
Ok, I am still a little bit hesitant in accepting these changes
considering that Kevin spent some time making a BMIPS multiplatform
kernel to work on 7425, 7435 [1]. Let's see how your changes look like,
and we can decide by then.
https://lwn.net/Articles/622947/
>
> -- Petri
>
> On Fri, Oct 10, 2014 at 12:46 PM, Petri Gynther <pgynther@...gle.com> wrote:
>> Hi Florian,
>>
>> On Fri, Oct 10, 2014 at 11:59 AM, Florian Fainelli <f.fainelli@...il.com> wrote:
>>> On 10/10/2014 11:35 AM, Petri Gynther wrote:
>>>> Broadcom 7xxx MIPS-based STB platforms do not use device trees.
>>>> Modify bcmgenet driver so that it can be used on those platforms.
>>>>
>>>> Signed-off-by: Petri Gynther <pgynther@...gle.com>
>>>> ---
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>>> index dbf524e..5191e3f 100644
>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>>> @@ -17,6 +17,17 @@
>>>> #include <linux/if_vlan.h>
>>>> #include <linux/phy.h>
>>>>
>>>> +struct bcmgenet_platform_data {
>>>> + void __iomem *base_reg;
>>>> + int irq0;
>>>> + int irq1;
>>>
>>> Why would these members here? The platform device should provide those
>>> as standard resources that the driver fetches using
>>> platform_get_resource() and platform_get_irq().
>>>
>>
>> I modeled this on struct bcmemac_platform_data that was used in the
>> legacy BRCMSTB code.
>> include/linux/brcmstb/brcmstb.h:
>>
>> struct bcmemac_platform_data {
>> /* used by the BSP code only */
>> uintptr_t base_reg;
>> int irq0;
>> int irq1;
>>
>> int phy_type;
>> int phy_id;
>> int phy_speed;
>> u8 macaddr[ETH_ALEN];
>> };
>>
>> The legacy BRCMSTB code stores all relevant GENET info in this struct
>> and then creates the resources from that info:
>>
>> static void __init brcm_register_genet(int id, struct bcmemac_platform_data *pd)
>> {
>> struct resource res[3];
>> struct platform_device *pdev;
>>
>> memset(&res, 0, sizeof(res));
>> res[0].start = BPHYSADDR(pd->base_reg);
>> res[0].end = BPHYSADDR(pd->base_reg + 0x4fff);
>> res[0].flags = IORESOURCE_MEM;
>>
>> res[1].start = res[1].end = pd->irq0;
>> res[1].flags = IORESOURCE_IRQ;
>>
>> res[2].start = res[2].end = pd->irq1;
>> res[2].flags = IORESOURCE_IRQ;
>>
>> brcm_alloc_macaddr(pd->macaddr);
>>
>> pdev = platform_device_alloc("bcmgenet", id);
>> platform_device_add_resources(pdev, res, 3);
>> platform_device_add_data(pdev, pd, sizeof(*pd));
>> platform_device_add(pdev);
>> }
>>
>>>> + int phy_type;
>>>> + int phy_addr;
>>>> + int phy_speed;
>>>> + u8 macaddr[ETH_ALEN];
>>>> + int genet_version;
>>>> +};
>>>
>>> I would rather we put this in include/linux/platform_data/bcmgenet.h
>>> where it belongs.
>>>
>>
>> I wasn't aware of the directory include/linux/platform_data/. Yes,
>> that's where this belongs.
>>
>>>> +
>>>> /* total number of Buffer Descriptors, same for Rx/Tx */
>>>> #define TOTAL_DESC 256
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>>> index 9ff799a..e5ff913 100644
>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>>> @@ -157,6 +157,21 @@ static void bcmgenet_mii_setup(struct net_device *dev)
>>>> phy_print_status(phydev);
>>>> }
>>>>
>>>> +static int bcmgenet_moca_fphy_update(struct net_device *dev,
>>>> + struct fixed_phy_status *status)
>>>> +{
>>>> + struct bcmgenet_priv *priv = netdev_priv(dev);
>>>> + struct phy_device *phydev = priv->phydev;
>>>> +
>>>> + /*
>>>> + * MoCA daemon updates phydev->autoneg to reflect the link status.
>>>> + * Update MoCA fixed PHY status accordingly, so that the PHY state
>>>> + * machine becomes aware of the real link status.
>>>> + */
>>>> + status->link = phydev->autoneg;
>>>> + return 0;
>>>> +}
>>>
>>> I don't want to see that in the upstream driver, please enable the link
>>> interrupts like I suggested before and do not use the autoneg field at
>>> all, which should require no MoCA daemon modifications.
>>>
>>
>> I added debug printk's to bcmgenet_isr0 to check on UMAC_IRQ_LINK_UP
>> and UMAC_IRQ_LINK_DOWN.
>> I am not getting those interrupts on eth1 (MoCA) port when coax is
>> removed/inserted.
>> But, they do work on eth0.
>>
>> I'll modify init_umac() to enable those interrupts for MoCA port and retest.
>>
>>> [snip]
>>>
>>>>
>>>> priv->phydev = phydev;
>>>> @@ -437,6 +464,104 @@ static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
>>>> return 0;
>>>> }
>>>>
>>>> +static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>>>> +{
>>>> + struct device *kdev = &priv->pdev->dev;
>>>> + struct bcmgenet_platform_data *pd = kdev->platform_data;
>>>> + struct mii_bus *mdio = priv->mii_bus;
>>>> + int phy_addr = pd->phy_addr;
>>>> + struct phy_device *phydev;
>>>> + int ret;
>>>> + int i;
>>>> +
>>>> + /* Disable automatic MDIO bus scan */
>>>> + mdio->phy_mask = ~0;
>>>> +
>>>> + /* Clear all the IRQ properties */
>>>> + if (mdio->irq)
>>>> + for (i = 0; i < PHY_MAX_ADDR; i++)
>>>> + mdio->irq[i] = PHY_POLL;
>>>> +
>>>> + /* Register the MDIO bus */
>>>> + ret = mdiobus_register(mdio);
>>>> + if (ret) {
>>>> + dev_err(kdev, "failed to register MDIO bus\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /*
>>>> + * bcmgenet_platform_data needs to pass a valid PHY address for
>>>> + * internal/external PHY or -1 for MoCA PHY.
>>>> + */
>>>> + if (phy_addr >= 0 && phy_addr < PHY_MAX_ADDR) {
>>>
>>> We do too much low-level PHY device handling, and since you already have
>>> the phy_type provided via platform_data, we can use that hint to do the
>>> following:
>>>
>>> 1) an internal or external PHY with MDIO accesses should leave the bus
>>> auto-probing on with the specified PHY address in the mdio bus phy_mask
>>>
>>> 2) a MoCA PHY or an external PHY with MDIO accesses disabled should use
>>> the fixed-0 MII bus instead.
>>>
>>> This would look like this:
>>>
>>> if (pd->phy_type != PHY_INTERFACE_MODE_MOCA || pd->mdio_enabled)
>>> mdio->phy_mask = ~(1 << pd->phy_addr);
>>>
>>> ...
>>> mdiobus_register()
>>>
>>> priv->phydev = mdio->bus->phy_map[pd->phy_addr];
>>>
>>> phydev->phy_flags |= mask;
>>>
>>> if (pd->phy_type == PHY_INTERFACE_MODE_MOCA || !pd->mdio_enabled)
>>> priv->phydev = fixed_phy_register(...);
>>>
>>> and in both cases, later on you do connect to the PHY device
>>>
>>> I can cook a patch to illustrate what I think this could look like since
>>> I realize using pseudo-code to explain might not be the best thing.
>>>
>>
>> I think I understand what you mean. I'll make a change.
>>
>>>> + /*
>>>> + * 10/100/1000 Ethernet port with external or internal PHY.
>>>> + */
>>>> + phydev = get_phy_device(mdio, phy_addr, false);
>>>> + if (!phydev || IS_ERR(phydev)) {
>>>> + dev_err(kdev, "failed to create PHY device\n");
>>>> + mdiobus_unregister(mdio);
>>>> + return 1;
>>>> + }
>>>> +
>>>> + phydev->irq = PHY_POLL;
>>>> +
>>>> + ret = phy_device_register(phydev);
>>>> + if (ret) {
>>>> + dev_err(kdev, "failed to register PHY device\n");
>>>> + phy_device_free(phydev);
>>>> + mdiobus_unregister(mdio);
>>>> + return 1;
>>>> + }
>>>> +
>>>> + priv->phydev = phydev;
>>>> + priv->phy_interface = pd->phy_type;
>>>> + } else {
>>>> + /*
>>>> + * MoCA port with no MDIO-accessible PHY.
>>>> + * We need to use 1000/HD fixed PHY to represent the link layer.
>>>> + * MoCA daemon interacts with this PHY via ethtool.
>>>> + */
>>>> + struct fixed_phy_status moca_fphy_status = {
>>>> + .link = 0,
>>>> + .duplex = 0,
>>>
>>> This should be DUPLEX_FULL here, the link between GENET and the MoCA
>>> Ethernet convergence layer is full-duplex by nature (despite we report
>>> the PHY being half-duplex, which is a mistake in the downstream driver),
>>> the MoCA medium on the coaxial cable is half-duplex though, but that is
>>> handled by the MoCA HW.
>>>
>>> NB: I had issues in the past using a half-duplex link with the MoCA
>>> ethernet convergence layer, causing various types of packet loss because
>>> we use a simplified signaling internally in the hardware.
>>>
>>
>> I picked this setting from 3.3 GENET driver. I'll test 1000/FULL on my
>> platform to see if it works.
>>
>>>> + .speed = 1000,
>>>> + .pause = 0,
>>>> + .asym_pause = 0,
>>>> + };
>>>> +
>>>> + phydev = fixed_phy_register(PHY_POLL, &moca_fphy_status, NULL);
>>>> + if (!phydev || IS_ERR(phydev)) {
>>>> + dev_err(kdev, "failed to register fixed PHY device\n");
>>>> + mdiobus_unregister(mdio);
>>>> + return 1;
>>>> + }
>>>> +
>>>> + phydev->autoneg = AUTONEG_DISABLE;
>>>> +
>>>> + ret = fixed_phy_set_link_update(phydev,
>>>> + bcmgenet_moca_fphy_update);
>>>> + if (ret) {
>>>> + dev_err(kdev, "failed to set fixed PHY link update\n");
>>>> + }
>>>
>>> Should not we propagate this error to the caller?
>>
>> Good catch. Yes.
>>
>>> --
>>> Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists