* Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
From: Terry Duncan @ 2019-08-13 21:15 UTC (permalink / raw)
To: Ben Wei, Tao Ren
Cc: Andrew Lunn, Jakub Kicinski, netdev@vger.kernel.org,
openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Samuel Mendoza-Jonas, David S.Miller, William Kennington
In-Reply-To: <CH2PR15MB3686B3A20A231FC111C42F40A3D20@CH2PR15MB3686.namprd15.prod.outlook.com>
On 8/13/19 12:52 PM, Ben Wei wrote:
>> On 8/13/19 9:31 AM, Terry Duncan wrote:
>>> Tao, in your new patch will it be possible to disable the setting of the BMC MAC? I would like to be able to send NCSI_OEM_GET_MAC perhaps with netlink (TBD) to get the system address without it affecting the BMC address.
>>>
>>> I was about to send patches to add support for the Intel adapters when I saw this thread.
>>> Thanks,
>>> Terry
>>
> Hi Terry,
> Do you plan to monitor and configure NIC through use space programs via netlink? I'm curious if you have additional use cases.
> I had planned to add some daemon in user space to monitor NIC through NC-SI, primarily to log AENs, check link status and retrieve NIC counters.
> We can collaborate on these if they align with your needs as well.
>
> Also about Intel NIC, do you not plan to derive system address from BMC MAC? Is the BMC MAC independent of system address?
>
> Thanks,
> -Ben
>
Hi Ben,
Intel has in the past programmed BMC MACs with an offset from the system
MAC address of the first shared interface. We have found this approach
causes problems and adds complexity when system interfaces / OCP cards
are added or removed or disabled in BIOS. Our approach in OpenBMC is to
keep this simple - provide means for the BMC MAC addresses to be set in
manufacturing and persist them.
We do also have some of the same use cases you mention.
Thanks,
Terry
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/4] bpf: support cloning sk storage on accept()
From: Daniel Borkmann @ 2019-08-13 21:12 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, bpf, davem, ast, Martin KaFai Lau,
Yonghong Song
In-Reply-To: <20190812175249.GF2820@mini-arch>
On 8/12/19 7:52 PM, Stanislav Fomichev wrote:
> On 08/12, Daniel Borkmann wrote:
>> On 8/9/19 6:10 PM, Stanislav Fomichev wrote:
>>> Add new helper bpf_sk_storage_clone which optionally clones sk storage
>>> and call it from sk_clone_lock.
>>>
>>> Cc: Martin KaFai Lau <kafai@fb.com>
>>> Cc: Yonghong Song <yhs@fb.com>
>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>> [...]
>>> +int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk)
>>> +{
>>> + struct bpf_sk_storage *new_sk_storage = NULL;
>>> + struct bpf_sk_storage *sk_storage;
>>> + struct bpf_sk_storage_elem *selem;
>>> + int ret;
>>> +
>>> + RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL);
>>> +
>>> + rcu_read_lock();
>>> + sk_storage = rcu_dereference(sk->sk_bpf_storage);
>>> +
>>> + if (!sk_storage || hlist_empty(&sk_storage->list))
>>> + goto out;
>>> +
>>> + hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
>>> + struct bpf_sk_storage_elem *copy_selem;
>>> + struct bpf_sk_storage_map *smap;
>>> + struct bpf_map *map;
>>> + int refold;
>>> +
>>> + smap = rcu_dereference(SDATA(selem)->smap);
>>> + if (!(smap->map.map_flags & BPF_F_CLONE))
>>> + continue;
>>> +
>>> + map = bpf_map_inc_not_zero(&smap->map, false);
>>> + if (IS_ERR(map))
>>> + continue;
>>> +
>>> + copy_selem = bpf_sk_storage_clone_elem(newsk, smap, selem);
>>> + if (!copy_selem) {
>>> + ret = -ENOMEM;
>>> + bpf_map_put(map);
>>> + goto err;
>>> + }
>>> +
>>> + if (new_sk_storage) {
>>> + selem_link_map(smap, copy_selem);
>>> + __selem_link_sk(new_sk_storage, copy_selem);
>>> + } else {
>>> + ret = sk_storage_alloc(newsk, smap, copy_selem);
>>> + if (ret) {
>>> + kfree(copy_selem);
>>> + atomic_sub(smap->elem_size,
>>> + &newsk->sk_omem_alloc);
>>> + bpf_map_put(map);
>>> + goto err;
>>> + }
>>> +
>>> + new_sk_storage = rcu_dereference(copy_selem->sk_storage);
>>> + }
>>> + bpf_map_put(map);
>>
>> The map get/put combination /under/ RCU read lock seems a bit odd to me, could
>> you exactly describe the race that this would be preventing?
> There is a race between sk storage release and sk storage clone.
> bpf_sk_storage_map_free uses synchronize_rcu to wait for all existing
> users to finish and the new ones are prevented via map's refcnt being
> zero; we need to do something like that for the clone.
> Martin suggested to use bpf_map_inc_not_zero/bpf_map_put.
> If I read everythin correctly, I think without map_inc/map_put we
> get the following race:
>
> CPU0 CPU1
>
> bpf_map_put
> bpf_sk_storage_map_free(smap)
> synchronize_rcu
>
> // no more users via bpf or
> // syscall, but clone
> // can still happen
>
> for each (bucket)
> selem_unlink
> selem_unlink_map(smap)
>
> // adding anything at
> // this point to the
> // bucket will leak
>
> rcu_read_lock
> tcp_v4_rcv
> tcp_v4_do_rcv
> // sk is lockless TCP_LISTEN
> tcp_v4_cookie_check
> tcp_v4_syn_recv_sock
> bpf_sk_storage_clone
> rcu_dereference(sk->sk_bpf_storage)
> selem_link_map(smap, copy)
> // adding new element to the
> // map -> leak
> rcu_read_unlock
>
> selem_unlink_sk
> sk->sk_bpf_storage = NULL
>
> synchronize_rcu
>
Makes sense, thanks for clarifying. Perhaps a small comment on top of
the bpf_map_inc_not_zero() would be great as well, so it's immediately
clear also from this location when reading the code why this is done.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net-next v2 02/12] net: stmmac: Prepare to add Split Header support
From: Jakub Kicinski @ 2019-08-13 21:11 UTC (permalink / raw)
To: Jose Abreu
Cc: netdev, Joao Pinto, Giuseppe Cavallaro, Alexandre Torgue,
David S. Miller, Maxime Coquelin, linux-stm32, linux-arm-kernel,
linux-kernel
In-Reply-To: <342007d6ac2b44db03d113e7e8bf0310caa77ea0.1565602974.git.joabreu@synopsys.com>
On Mon, 12 Aug 2019 11:44:01 +0200, Jose Abreu wrote:
> In order to add Split Header support, stmmac_rx() needs to take into
> account that packet may be split accross multiple descriptors.
>
> Refactor the logic of this function in order to support this scenario.
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>
> ---
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Jose Abreu <joabreu@synopsys.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 6 +
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 149 +++++++++++++---------
> 2 files changed, 95 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 80276587048a..56158e1448ac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -74,6 +74,12 @@ struct stmmac_rx_queue {
> u32 rx_zeroc_thresh;
> dma_addr_t dma_rx_phy;
> u32 rx_tail_addr;
> + unsigned int state_saved;
> + struct {
> + struct sk_buff *skb;
> + unsigned int len;
> + unsigned int error;
> + } state;
> };
>
> struct stmmac_channel {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b2e5f4ecd551..a093eb4ec275 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3353,9 +3353,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> {
> struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
> struct stmmac_channel *ch = &priv->channel[queue];
> + unsigned int count = 0, error = 0, len = 0;
> + int status = 0, coe = priv->hw->rx_csum;
> unsigned int next_entry = rx_q->cur_rx;
> - int coe = priv->hw->rx_csum;
> - unsigned int count = 0;
> + struct sk_buff *skb = NULL;
>
> if (netif_msg_rx_status(priv)) {
> void *rx_head;
> @@ -3369,9 +3370,27 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> stmmac_display_ring(priv, rx_head, DMA_RX_SIZE, true);
> }
> while (count < limit) {
> + enum pkt_hash_types hash_type;
> struct stmmac_rx_buffer *buf;
> + unsigned int prev_len = 0;
> struct dma_desc *np, *p;
> - int entry, status;
> + int entry;
> + u32 hash;
> +
> + if (!count && rx_q->state_saved) {
> + skb = rx_q->state.skb;
> + error = rx_q->state.error;
> + len = rx_q->state.len;
> + } else {
> + rx_q->state_saved = false;
> + skb = NULL;
> + error = 0;
> + len = 0;
> + }
> +
> +read_again:
> + if (count >= limit)
> + break;
Is this stopping the NAPI poll once @limit descriptors were seen?
It should probably be okay to ignore the limit until you get a full
frame? I'd think it'd be best to finish up the frame while the state
is hot in the CPU cache.. WDYT?
> entry = next_entry;
> buf = &rx_q->buf_pool[entry];
> @@ -3407,28 +3426,24 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> page_pool_recycle_direct(rx_q->page_pool, buf->page);
> priv->dev->stats.rx_errors++;
> buf->page = NULL;
> + error = 1;
> + }
> +
> + if (unlikely(error & (status & rx_not_ls)))
Looks suspicious - sure this is supposed to be error & (status & bla)
and not error && ... ?
> + goto read_again;
> + if (unlikely(error)) {
> + if (skb)
> + dev_kfree_skb(skb);
> + continue;
> + }
> +
> + /* Buffer is good. Go on. */
> +
> + if (likely(status & rx_not_ls)) {
> + len += priv->dma_buf_sz;
> } else {
> - enum pkt_hash_types hash_type;
> - struct sk_buff *skb;
> - unsigned int des;
> - int frame_len;
> - u32 hash;
> -
> - stmmac_get_desc_addr(priv, p, &des);
> - frame_len = stmmac_get_rx_frame_len(priv, p, coe);
> -
> - /* If frame length is greater than skb buffer size
> - * (preallocated during init) then the packet is
> - * ignored
> - */
> - if (frame_len > priv->dma_buf_sz) {
> - if (net_ratelimit())
> - netdev_err(priv->dev,
> - "len %d larger than size (%d)\n",
> - frame_len, priv->dma_buf_sz);
> - priv->dev->stats.rx_length_errors++;
> - continue;
> - }
> + prev_len = len;
> + len = stmmac_get_rx_frame_len(priv, p, coe);
>
> /* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
> * Type frames (LLC/LLC-SNAP)
> @@ -3439,57 +3454,71 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> */
> if (unlikely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
> unlikely(status != llc_snap))
> - frame_len -= ETH_FCS_LEN;
> -
> - if (netif_msg_rx_status(priv)) {
> - netdev_dbg(priv->dev, "\tdesc: %p [entry %d] buff=0x%x\n",
> - p, entry, des);
> - netdev_dbg(priv->dev, "frame size %d, COE: %d\n",
> - frame_len, status);
> - }
> + len -= ETH_FCS_LEN;
> + }
>
> - skb = netdev_alloc_skb_ip_align(priv->dev, frame_len);
> - if (unlikely(!skb)) {
> + if (!skb) {
> + skb = netdev_alloc_skb_ip_align(priv->dev, len);
Since you're in NAPI call perhaps something like napi_alloc_skb() could
speed things up a little? But please also see below..
> + if (!skb) {
> priv->dev->stats.rx_dropped++;
> continue;
> }
>
> - dma_sync_single_for_cpu(priv->device, buf->addr,
> - frame_len, DMA_FROM_DEVICE);
> + dma_sync_single_for_cpu(priv->device, buf->addr, len,
> + DMA_FROM_DEVICE);
> skb_copy_to_linear_data(skb, page_address(buf->page),
> - frame_len);
> - skb_put(skb, frame_len);
> + len);
> + skb_put(skb, len);
>
> - if (netif_msg_pktdata(priv)) {
> - netdev_dbg(priv->dev, "frame received (%dbytes)",
> - frame_len);
> - print_pkt(skb->data, frame_len);
> - }
> + /* Data payload copied into SKB, page ready for recycle */
> + page_pool_recycle_direct(rx_q->page_pool, buf->page);
> + buf->page = NULL;
> + } else {
> + unsigned int buf_len = len - prev_len;
>
> - stmmac_get_rx_hwtstamp(priv, p, np, skb);
> + if (likely(status & rx_not_ls))
> + buf_len = priv->dma_buf_sz;
>
> - stmmac_rx_vlan(priv->dev, skb);
> + dma_sync_single_for_cpu(priv->device, buf->addr,
> + buf_len, DMA_FROM_DEVICE);
> + skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> + buf->page, 0, buf_len,
> + priv->dma_buf_sz);
>
> - skb->protocol = eth_type_trans(skb, priv->dev);
> + /* Data payload appended into SKB */
> + page_pool_release_page(rx_q->page_pool, buf->page);
> + buf->page = NULL;
> + }
>
> - if (unlikely(!coe))
> - skb_checksum_none_assert(skb);
> - else
> - skb->ip_summed = CHECKSUM_UNNECESSARY;
> + if (likely(status & rx_not_ls))
> + goto read_again;
>
> - if (!stmmac_get_rx_hash(priv, p, &hash, &hash_type))
> - skb_set_hash(skb, hash, hash_type);
> + /* Got entire packet into SKB. Finish it. */
>
> - skb_record_rx_queue(skb, queue);
> - napi_gro_receive(&ch->rx_napi, skb);
> + stmmac_get_rx_hwtstamp(priv, p, np, skb);
> + stmmac_rx_vlan(priv->dev, skb);
> + skb->protocol = eth_type_trans(skb, priv->dev);
>
> - /* Data payload copied into SKB, page ready for recycle */
> - page_pool_recycle_direct(rx_q->page_pool, buf->page);
> - buf->page = NULL;
> + if (unlikely(!coe))
> + skb_checksum_none_assert(skb);
> + else
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> - priv->dev->stats.rx_packets++;
> - priv->dev->stats.rx_bytes += frame_len;
> - }
> + if (!stmmac_get_rx_hash(priv, p, &hash, &hash_type))
> + skb_set_hash(skb, hash, hash_type);
> +
> + skb_record_rx_queue(skb, queue);
> + napi_gro_receive(&ch->rx_napi, skb);
Did you look into using napi_gro_frags() family of APIs?
I think Eric said those are more efficient from GRO perspective..
> + priv->dev->stats.rx_packets++;
> + priv->dev->stats.rx_bytes += len;
> + }
> +
> + if (status & rx_not_ls) {
> + rx_q->state_saved = true;
> + rx_q->state.skb = skb;
> + rx_q->state.error = error;
> + rx_q->state.len = len;
> }
>
> stmmac_rx_refill(priv, queue);
^ permalink raw reply
* Re: [PATCH net-next v2 01/12] net: stmmac: Get correct timestamp values from XGMAC
From: Jakub Kicinski @ 2019-08-13 20:57 UTC (permalink / raw)
To: Jose Abreu
Cc: netdev, Joao Pinto, Giuseppe Cavallaro, Alexandre Torgue,
David S. Miller, Maxime Coquelin, linux-stm32, linux-arm-kernel,
linux-kernel
In-Reply-To: <195f374a0b46e5e65a691742fc2dbeffacfaf148.1565602974.git.joabreu@synopsys.com>
On Mon, 12 Aug 2019 11:44:00 +0200, Jose Abreu wrote:
> TX Timestamp in XGMAC comes from MAC instead of descriptors. Implement
> this in a new callback.
>
> Also, RX Timestamp in XGMAC must be cheked against corruption and we need
> a barrier to make sure that descriptor fields are read correctly.
>
> Changes from v1:
> - Rework the get timestamp function (David)
>
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
The barrier sounds like it might be a bug fix, does it occur in he wild?
> @@ -113,13 +119,11 @@ static int dwxgmac2_get_rx_timestamp_status(void *desc, void *next_desc,
> unsigned int rdes3 = le32_to_cpu(p->des3);
> int ret = -EBUSY;
>
> - if (likely(rdes3 & XGMAC_RDES3_CDA)) {
> + if (likely(rdes3 & XGMAC_RDES3_CDA))
> ret = dwxgmac2_rx_check_timestamp(next_desc);
> - if (ret)
> - return ret;
> - }
> -
> - return ret;
> + if (!ret)
> + return 1;
> + return 0;
nit:
return !ret;
> }
>
> static void dwxgmac2_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
^ permalink raw reply
* Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
From: Terry Duncan @ 2019-08-13 20:54 UTC (permalink / raw)
To: Tao Ren
Cc: Andrew Lunn, Jakub Kicinski, netdev@vger.kernel.org,
openbmc@lists.ozlabs.org, Ben Wei, linux-kernel@vger.kernel.org,
Samuel Mendoza-Jonas, David S.Miller, William Kennington
In-Reply-To: <faa1b3c9-9ba3-0fff-e1d4-f6dddb60c52c@fb.com>
On 8/13/19 11:28 AM, Tao Ren wrote:
> On 8/13/19 9:31 AM, Terry Duncan wrote:
>> Tao, in your new patch will it be possible to disable the setting of the BMC MAC? I would like to be able to send NCSI_OEM_GET_MAC perhaps with netlink (TBD) to get the system address without it affecting the BMC address.
>>
>> I was about to send patches to add support for the Intel adapters when I saw this thread.
>>
>> Thanks,
>>
>> Terry
>
> Hi Terry,
>
> Sounds like you are planning to configure BMC MAC address from user space via netlink? Ben Wei <benwei@fb.com> started a thread "Out-of-band NIC management" in openbmc community for NCSI management using netlink, and you may follow up with him for details.
>
> I haven't decided what to do in my v2 patch: maybe using device tree, maybe moving the logic to uboot, and I'm also evaluating the netlink option. But it shouldn't impact your patch, because you can disable NCSI_OEM_GET_MAC option from your config file.
Thanks Tao. I see now that disabling the NCSI_OEM_GET_MAC option will do
what I want.
Best,
Terry
^ permalink raw reply
* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
From: Matthias Kaehlcke @ 2019-08-13 20:46 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190813201411.GL15047@lunn.ch>
On Tue, Aug 13, 2019 at 10:14:11PM +0200, Andrew Lunn wrote:
> > +static int rtl8211e_config_led(struct phy_device *phydev, int led,
> > + struct phy_led_config *cfg)
> > +{
> > + u16 lacr_bits = 0, lcr_bits = 0;
> > + int oldpage, ret;
> > +
>
> You should probably check that led is 0 or 1.
Actually this PHY has up to 3 LEDs, but yes, good point to validate
the parameter. I'll wait a day or two for if there is other feedback
and send a new version with the check.
> Otherwise this looks good.
Thanks for the reviews!
Matthias
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mv88e6xxx: check for mode change in port_setup_mac
From: Marek Behun @ 2019-08-13 20:45 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, Vivien Didelot, Heiner Kallweit
In-Reply-To: <20190813203234.GO15047@lunn.ch>
On Tue, 13 Aug 2019 22:32:34 +0200
Andrew Lunn <andrew@lunn.ch> wrote:
> On Tue, Aug 13, 2019 at 07:12:43PM +0200, Marek Behún wrote:
> > @@ -598,12 +599,49 @@ int mv88e6352_port_link_state(struct mv88e6xxx_chip *chip, int port,
> > struct phylink_link_state *state)
> > {
> > int err;
> > - u16 reg;
> > + u16 reg, mac;
> >
> > err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, ®);
> > if (err)
> > return err;
>
> Hi Marek
>
> It seems a bit off putting this block of code here, after reading
> MV88E6XXX_PORT_STS but before using the value. You don't need STS to
> determine the interface mode.
>
> If you keep the code together, you can then reuse reg, rather than add
> mac.
>
> Apart from that, this looks O.K.
>
> Andrew
Hi Andrew,
you are right, I shall rewrite this.
Thank you.
Marek
^ permalink raw reply
* Re: [PATCH net] netdevsim: Restore per-network namespace accounting for fib entries
From: Jiri Pirko @ 2019-08-13 20:37 UTC (permalink / raw)
To: David Miller; +Cc: dsahern, netdev, dsahern
In-Reply-To: <20190813.104054.140346412563349218.davem@davemloft.net>
Tue, Aug 13, 2019 at 07:40:54PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Tue, 13 Aug 2019 09:14:45 +0200
>
>> Mon, Aug 12, 2019 at 05:28:02PM CEST, davem@davemloft.net wrote:
>>>From: Jiri Pirko <jiri@resnulli.us>
>>>Date: Mon, 12 Aug 2019 10:36:35 +0200
>>>
>>>> I understand it with real devices, but dummy testing device, who's
>>>> purpose is just to test API. Why?
>>>
>>>Because you'll break all of the wonderful testing infrastructure
>>>people like David have created.
>>
>> Are you referring to selftests? There is no such test there :(
>> But if it would be, could implement the limitations
>> properly (like using cgroups), change the tests and remove this
>> code from netdevsim?
>
>What about older kernels? Can't you see how illogical this is?
Not really, since this is a dummy testing device. Not like we would
break anybody. Well, I changed instantiation of netdevsim from rtnetlink
to sysfs, that is even bigger breakage, nobody cared (of course).
Well sure we can comeup with netdevsim2, netdevsimN, to maintain backward
compatibility of netdevsim. I find it ridiculous. But anyway, I don't
really care that much. I resign as this seems futile :(
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mv88e6xxx: check for mode change in port_setup_mac
From: Andrew Lunn @ 2019-08-13 20:32 UTC (permalink / raw)
To: Marek Behún; +Cc: netdev, Vivien Didelot, Heiner Kallweit
In-Reply-To: <20190813171243.27898-1-marek.behun@nic.cz>
On Tue, Aug 13, 2019 at 07:12:43PM +0200, Marek Behún wrote:
> @@ -598,12 +599,49 @@ int mv88e6352_port_link_state(struct mv88e6xxx_chip *chip, int port,
> struct phylink_link_state *state)
> {
> int err;
> - u16 reg;
> + u16 reg, mac;
>
> err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, ®);
> if (err)
> return err;
Hi Marek
It seems a bit off putting this block of code here, after reading
MV88E6XXX_PORT_STS but before using the value. You don't need STS to
determine the interface mode.
If you keep the code together, you can then reuse reg, rather than add
mac.
Apart from that, this looks O.K.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: phy: add phy_speed_down_core and phy_resolve_min_speed
From: Andrew Lunn @ 2019-08-13 20:16 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <d0c44f84-d441-9d4e-96e1-d8abb7c0e508@gmail.com>
On Mon, Aug 12, 2019 at 11:51:27PM +0200, Heiner Kallweit wrote:
> phy_speed_down_core provides most of the functionality for
> phy_speed_down. It makes use of new helper phy_resolve_min_speed that is
> based on the sorting of the settings[] array. In certain cases it may be
> helpful to be able to exclude legacy half duplex modes, therefore
> prepare phy_resolve_min_speed() for it.
>
> v2:
> - rename __phy_speed_down to phy_speed_down_core
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] net: phy: add __set_linkmode_max_speed
From: Andrew Lunn @ 2019-08-13 20:16 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <4c77b801-6005-834c-da0e-f32847961f81@gmail.com>
On Mon, Aug 12, 2019 at 11:50:30PM +0200, Heiner Kallweit wrote:
> We will need the functionality of __set_linkmode_max_speed also for
> linkmode bitmaps other than phydev->supported. Therefore split it.
>
> v2:
> - remove unused parameter from __set_linkmode_max_speed
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
From: Andrew Lunn @ 2019-08-13 20:14 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190813191147.19936-5-mka@chromium.org>
> +static int rtl8211e_config_led(struct phy_device *phydev, int led,
> + struct phy_led_config *cfg)
> +{
> + u16 lacr_bits = 0, lcr_bits = 0;
> + int oldpage, ret;
> +
You should probably check that led is 0 or 1.
Otherwise this looks good.
Andrew
^ permalink raw reply
* Re: [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages
From: Andrew Lunn @ 2019-08-13 20:09 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190813191147.19936-4-mka@chromium.org>
On Tue, Aug 13, 2019 at 12:11:46PM -0700, Matthias Kaehlcke wrote:
> Some RTL8211x PHYs have extension pages, which can be accessed
> after selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a helper for
> selecting an ext page. Use rtl8211x_modify_ext_paged() in
> rtl8211e_config_init() instead of doing things 'manually'.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT
From: Andrew Lunn @ 2019-08-13 19:56 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190813191147.19936-3-mka@chromium.org>
On Tue, Aug 13, 2019 at 12:11:45PM -0700, Matthias Kaehlcke wrote:
> For PHYs with a device tree node look for LED trigger configuration
> using the generic binding, if it exists try to apply it via the new
> driver hook .config_led.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next] net: phy: modify assignment to OR for dev_flags in phy_attach_direct
From: Florian Fainelli @ 2019-08-13 19:54 UTC (permalink / raw)
To: Tao Ren, Andrew Lunn, Heiner Kallweit, David S . Miller,
Vladimir Oltean, Arun Parameswaran, Justin Chen, netdev,
linux-kernel, openbmc
In-Reply-To: <20190805185551.3140564-1-taoren@fb.com>
On 8/5/19 11:55 AM, Tao Ren wrote:
> Modify the assignment to OR when dealing with phydev->dev_flags in
> phy_attach_direct function, and this is to make sure dev_flags set in
> driver's probe callback won't be lost.
As Andrew pointed out already, this probably needs to be reworked, but
for now this looks reasonable:
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
From: Andrew Lunn @ 2019-08-13 19:54 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
Heiner Kallweit, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <20190813191147.19936-2-mka@chromium.org>
On Tue, Aug 13, 2019 at 12:11:44PM -0700, Matthias Kaehlcke wrote:
> The LED behavior of some Ethernet PHYs is configurable. Add an
> optional 'leds' subnode with a child node for each LED to be
> configured. The binding aims to be compatible with the common
> LED binding (see devicetree/bindings/leds/common.txt).
>
> A LED can be configured to be:
>
> - 'on' when a link is active, some PHYs allow configuration for
> certain link speeds
> speeds
> - 'off'
> - blink on RX/TX activity, some PHYs allow configuration for
> certain link speeds
>
> For the configuration to be effective it needs to be supported by
> the hardware and the corresponding PHY driver.
>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next] mcast: ensure L-L IPv6 packets are accepted by bridge
From: Ido Schimmel @ 2019-08-13 19:53 UTC (permalink / raw)
To: Patrick Ruddy; +Cc: netdev, roopa, nikolay, linus.luessing
In-Reply-To: <20190813141804.20515-1-pruddy@vyatta.att-mail.com>
+ Bridge maintainers, Linus
On Tue, Aug 13, 2019 at 03:18:04PM +0100, Patrick Ruddy wrote:
> At present only all-nodes IPv6 multicast packets are accepted by
> a bridge interface that is not in multicast router mode. Since
> other protocols can be running in the absense of multicast
> forwarding e.g. OSPFv3 IPv6 ND. Change the test to allow
> all of the FFx2::/16 range to be accepted when not in multicast
> router mode. This aligns the code with IPv4 link-local reception
> and RFC4291
Can you please quote the relevant part from RFC 4291?
>
> Signed-off-by: Patrick Ruddy <pruddy@vyatta.att-mail.com>
> ---
> include/net/addrconf.h | 15 +++++++++++++++
> net/bridge/br_multicast.c | 2 +-
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index becdad576859..05b42867e969 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -434,6 +434,21 @@ static inline void addrconf_addr_solict_mult(const struct in6_addr *addr,
> htonl(0xFF000000) | addr->s6_addr32[3]);
> }
>
> +/*
> + * link local multicast address range ffx2::/16 rfc4291
> + */
> +static inline bool ipv6_addr_is_ll_mcast(const struct in6_addr *addr)
> +{
> +#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> + __be64 *p = (__be64 *)addr;
> + return ((p[0] & cpu_to_be64(0xff0f000000000000UL))
> + ^ cpu_to_be64(0xff02000000000000UL)) == 0UL;
> +#else
> + return ((addr->s6_addr32[0] & htonl(0xff0f0000)) ^
> + htonl(0xff020000)) == 0;
> +#endif
> +}
> +
> static inline bool ipv6_addr_is_ll_all_nodes(const struct in6_addr *addr)
> {
> #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 9b379e110129..ed3957381fa2 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1664,7 +1664,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
> err = ipv6_mc_check_mld(skb);
>
> if (err == -ENOMSG) {
> - if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
> + if (!ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
> BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
IIUC, you want IPv6 link-local packets to be locally received, but this
also changes how these packets are flooded. RFC 4541 says that packets
addressed to the all hosts address are a special case and should be
forwarded to all ports:
"In IPv6, the data forwarding rules are more straight forward because MLD is
mandated for addresses with scope 2 (link-scope) or greater. The only exception
is the address FF02::1 which is the all hosts link-scope address for which MLD
messages are never sent. Packets with the all hosts link-scope address should
be forwarded on all ports."
Maybe you want something like:
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 09b1dd8cd853..9f312a73f61c 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -132,7 +132,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
br_multicast_querier_exists(br, eth_hdr(skb))) {
if ((mdst && mdst->host_joined) ||
- br_multicast_is_router(br)) {
+ br_multicast_is_router(br) ||
+ BR_INPUT_SKB_CB_LOCAL_RECEIVE(skb)) {
local_rcv = true;
br->dev->stats.multicast++;
}
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 9b379e110129..f03cecf6174e 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1667,6 +1667,9 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
if (!ipv6_addr_is_ll_all_nodes(&ipv6_hdr(skb)->daddr))
BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
+ if (ipv6_addr_is_ll_mcast(&ipv6_hdr(skb)->daddr))
+ BR_INPUT_SKB_CB(skb)->local_receive = 1;
+
if (ipv6_addr_is_all_snoopers(&ipv6_hdr(skb)->daddr)) {
err = br_ip6_multicast_mrd_rcv(br, port, skb);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index b7a4942ff1b3..d76394ca4059 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -426,6 +426,7 @@ struct br_input_skb_cb {
#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
u8 igmp;
u8 mrouters_only:1;
+ u8 local_receive:1;
#endif
u8 proxyarp_replied:1;
u8 src_port_isolated:1;
@@ -445,8 +446,10 @@ struct br_input_skb_cb {
#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
# define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb) (BR_INPUT_SKB_CB(__skb)->mrouters_only)
+# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb) (BR_INPUT_SKB_CB(__skb)->local_receive)
#else
# define BR_INPUT_SKB_CB_MROUTERS_ONLY(__skb) (0)
+# define BR_INPUT_SKB_CB_LOCAL_RECEIVE(__skb) (0)
#endif
#define br_printk(level, br, format, args...) \
^ permalink raw reply related
* Re: [PATCH net-next,v4 08/12] drivers: net: use flow block API
From: Pablo Neira Ayuso @ 2019-08-13 19:51 UTC (permalink / raw)
To: Edward Cree; +Cc: netdev, netfilter-devel
In-Reply-To: <75eec70e-60de-e33b-aea0-be595ca625f4@solarflare.com>
On Mon, Aug 12, 2019 at 06:50:09PM +0100, Edward Cree wrote:
> On 09/07/2019 21:55, Pablo Neira Ayuso wrote:
> > This patch updates flow_block_cb_setup_simple() to use the flow block API.
> > Several drivers are also adjusted to use it.
> >
> > This patch introduces the per-driver list of flow blocks to account for
> > blocks that are already in use.
> >
> > Remove tc_block_offload alias.
> >
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> > v4: fix typo in list in nfp driver - Jakub Kicinski.
> > Move driver_list handling to the driver code, this list is transitional,
> > until drivers are updated to support multiple subsystems. No more
> > driver_list handling from core.
>
> Pablo, can you explain (because this commit message doesn't) why these per-
> driver lists are needed, and what the information/state is that has module
> (rather than, say, netdevice) scope?
The idea is to update drivers to support one flow_block per subsystem,
one for ethtool, one for tc, and so on. So far, existing drivers only
allow for binding one single flow_block to one of the existing
subsystems. So this limitation applies at driver level.
^ permalink raw reply
* Re: [PATCH net] net: dsa: mv88e6xxx: drop adjust_link to enabled phylink
From: Florian Fainelli @ 2019-08-13 19:50 UTC (permalink / raw)
To: Hubert Feurstein, Andrew Lunn
Cc: netdev, linux-kernel, Vivien Didelot, David S. Miller
In-Reply-To: <CAFfN3gX6_dvAkRqRuXdR_+nfsFyBd2UNSzYo1H3am49xyb-hBQ@mail.gmail.com>
On 8/5/19 1:49 AM, Hubert Feurstein wrote:
> Hi Andrew,
>
> It looks like some work is still needed in b53_phylink_mac_config to
> take over the
> functionality of the current adjust_link implementation.
Indeed, I will look into it in the next few weeks.
--
Florian
^ permalink raw reply
* Re: general protection fault in tls_write_space
From: John Fastabend @ 2019-08-13 19:30 UTC (permalink / raw)
To: Jakub Kicinski, John Fastabend
Cc: Hillf Danton, syzbot, aviadye, borisp, daniel, davejwatson, davem,
linux-kernel, netdev, oss-drivers, syzkaller-bugs, willemb
In-Reply-To: <20190813115948.5f57b272@cakuba.netronome.com>
Jakub Kicinski wrote:
> On Tue, 13 Aug 2019 11:30:00 -0700, John Fastabend wrote:
> > Jakub Kicinski wrote:
> > > On Tue, 13 Aug 2019 10:17:06 -0700, John Fastabend wrote:
> > > > > Followup of commit 95fa145479fb
> > > > > ("bpf: sockmap/tls, close can race with map free")
> > > > >
> > > > > --- a/net/tls/tls_main.c
> > > > > +++ b/net/tls/tls_main.c
> > > > > @@ -308,6 +308,9 @@ static void tls_sk_proto_close(struct so
> > > > > if (free_ctx)
> > > > > icsk->icsk_ulp_data = NULL;
> > > > > sk->sk_prot = ctx->sk_proto;
> > > > > + /* tls will go; restore sock callback before enabling bh */
> > > > > + if (sk->sk_write_space == tls_write_space)
> > > > > + sk->sk_write_space = ctx->sk_write_space;
> > > > > write_unlock_bh(&sk->sk_callback_lock);
> > > > > release_sock(sk);
> > > > > if (ctx->tx_conf == TLS_SW)
> > > >
> > > > Hi Hillf,
> > > >
> > > > We need this patch (although slightly updated for bpf tree) do
> > > > you want to send it? Otherwise I can. We should only set this if
> > > > TX path was enabled otherwise we null it. Checking against
> > > > tls_write_space seems best to me as well.
> > > >
> > > > Against bpf this patch should fix it.
> > > >
> > > > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > > > index ce6ef56a65ef..43252a801c3f 100644
> > > > --- a/net/tls/tls_main.c
> > > > +++ b/net/tls/tls_main.c
> > > > @@ -308,7 +308,8 @@ static void tls_sk_proto_close(struct sock *sk, long timeout)
> > > > if (free_ctx)
> > > > icsk->icsk_ulp_data = NULL;
> > > > sk->sk_prot = ctx->sk_proto;
> > > > - sk->sk_write_space = ctx->sk_write_space;
> > > > + if (sk->sk_write_space == tls_write_space)
> > > > + sk->sk_write_space = ctx->sk_write_space;
> > > > write_unlock_bh(&sk->sk_callback_lock);
> > > > release_sock(sk);
> > > > if (ctx->tx_conf == TLS_SW)
> > >
> > > This is already in net since Friday:
> >
> > Don't we need to guard that with an
> >
> > if (sk->sk_write_space == tls_write_space)
> >
> > or something similar? Where is ctx->sk_write_space set in the rx only
> > case? In do_tls_setsockop_conf() we have this block
> >
> > if (tx) {
> > ctx->sk_write_space = sk->sk_write_space;
> > sk->sk_write_space = tls_write_space;
> > } else {
> > sk->sk_socket->ops = &tls_sw_proto_ops;
> > }
> >
> > which makes me think ctx->sk_write_space may not be set correctly in
> > all cases.
>
> Ah damn, you're right I remember looking at that but then I went down
> the rabbit hole of trying to repro and forgot :/
>
> Do you want to send an incremental change?
Sure I'll send something out this afternoon.
^ permalink raw reply
* [PATCH v6 0/4] net: phy: Add support for DT configuration of PHY LEDs and use it for RTL8211E
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
This series adds a generic binding to configure PHY LEDs through
the device tree, and phylib support for reading the information
from the DT. PHY drivers that support the generic binding should
implement the new hook .config_led.
Enable DT configuration of the RTL8211E LEDs by implementing the
.config_led hook of the driver. Certain registers of the RTL8211E
can only be accessed through a vendor specific extended page
mechanism. Extended pages need to be accessed for the LED
configuration. This series adds helpers to facilitate accessing
extended pages.
Matthias Kaehlcke (4):
dt-bindings: net: phy: Add subnode for LED configuration
net: phy: Add support for generic LED configuration through the DT
net: phy: realtek: Add helpers for accessing RTL8211x extension pages
net: phy: realtek: Add LED configuration support for RTL8211E
.../devicetree/bindings/net/ethernet-phy.yaml | 59 ++++++++
drivers/net/phy/phy_device.c | 72 +++++++++
drivers/net/phy/realtek.c | 137 ++++++++++++++++--
include/linux/phy.h | 22 +++
4 files changed, 275 insertions(+), 15 deletions(-)
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply
* [PATCH v6 4/4] net: phy: realtek: Add LED configuration support for RTL8211E
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190813191147.19936-1-mka@chromium.org>
Add a .config_led hook which is called by the PHY core when
configuration data for a PHY LED is available. Each LED can be
configured to be solid 'off, solid 'on' for certain (or all)
link speeds or to blink on RX/TX activity.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- return -EOPNOTSUPP if trigger is not supported, don't log warning
- don't log errors if MDIO ops fail, this is rare and the phy_device
will log a warning
- added parentheses around macro argument used in arithmetics to
avoid possible operator precedence issues
- minor formatting changes
Changes in v5:
- use 'config_leds' driver callback instead of requesting the DT
configuration
- added support for trigger 'none'
- always disable EEE LED mode when a LED is configured. We have no
device data struct to keep track of its state, the number of LEDs
is limited, so the overhead of disabling it multiple times (once for
each LED that is configured) during initialization is negligible
- print warning when disabling EEE LED mode fails
- updated commit message (previous subject was 'net: phy: realtek:
configure RTL8211E LEDs')
Changes in v4:
- use the generic PHY LED binding
- keep default/current configuration if none is specified
- added rtl8211e_disable_eee_led_mode()
- was previously in separate patch, however since we always want to
disable EEE LED mode when a LED configuration is specified it makes
sense to just add the function here.
- don't call phy_restore_page() in rtl8211e_config_leds() if
selection of the extended page failed.
- use phydev_warn() instead of phydev_err() if LED configuration
fails since we don't bail out
- use hex number to specify page for consistency
- add hex number to comment about ext page 44 to facilitate searching
Changes in v3:
- sanity check led-modes values
- set LACR bits in a more readable way
- use phydev_err() instead of dev_err()
- log an error if LED configuration fails
Changes in v2:
- patch added to the series
---
drivers/net/phy/realtek.c | 90 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 89 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a5b3708dc4d8..2bca3b91d43d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,8 +9,9 @@
* Copyright (c) 2004 Freescale Semiconductor, Inc.
*/
#include <linux/bitops.h>
-#include <linux/phy.h>
+#include <linux/bits.h>
#include <linux/module.h>
+#include <linux/phy.h>
#define RTL821x_PHYSR 0x11
#define RTL821x_PHYSR_DUPLEX BIT(13)
@@ -26,6 +27,19 @@
#define RTL821x_EXT_PAGE_SELECT 0x1e
#define RTL821x_PAGE_SELECT 0x1f
+/* RTL8211E page 5 */
+#define RTL8211E_EEE_LED_MODE1 0x05
+#define RTL8211E_EEE_LED_MODE2 0x06
+
+/* RTL8211E extension page 44 (0x2c) */
+#define RTL8211E_LACR 0x1a
+#define RLT8211E_LACR_LEDACTCTRL_SHIFT 4
+#define RTL8211E_LCR 0x1c
+
+#define LACR_MASK(led) BIT(4 + (led))
+#define LCR_MASK(led) GENMASK(((led) * 4) + 2,\
+ (led) * 4)
+
#define RTL8211F_INSR 0x1d
#define RTL8211F_TX_DELAY BIT(8)
@@ -83,6 +97,79 @@ static int rtl8211x_modify_ext_paged(struct phy_device *phydev, int page,
return phy_restore_page(phydev, oldpage, ret);
}
+static void rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
+{
+ int oldpage;
+ int err = 0;
+
+ oldpage = phy_select_page(phydev, 5);
+ if (oldpage < 0)
+ goto out;
+
+ /* write magic values to disable EEE LED mode */
+ err = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
+ if (err)
+ goto out;
+
+ err = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
+
+out:
+ if (err)
+ phydev_warn(phydev, "failed to disable EEE LED mode: %d\n",
+ err);
+
+ phy_restore_page(phydev, oldpage, err);
+}
+
+static int rtl8211e_config_led(struct phy_device *phydev, int led,
+ struct phy_led_config *cfg)
+{
+ u16 lacr_bits = 0, lcr_bits = 0;
+ int oldpage, ret;
+
+ switch (cfg->trigger.t) {
+ case PHY_LED_TRIGGER_LINK:
+ lcr_bits = 7 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_LINK_10M:
+ lcr_bits = 1 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_LINK_100M:
+ lcr_bits = 2 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_LINK_1G:
+ lcr_bits |= 4 << (led * 4);
+ break;
+
+ case PHY_LED_TRIGGER_NONE:
+ break;
+
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ if (cfg->trigger.activity)
+ lacr_bits = BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + led);
+
+ rtl8211e_disable_eee_led_mode(phydev);
+
+ oldpage = rtl8211x_select_ext_page(phydev, 0x2c);
+ if (oldpage < 0)
+ return oldpage;
+
+ ret = __phy_modify(phydev, RTL8211E_LACR, LACR_MASK(led), lacr_bits);
+ if (ret)
+ goto err;
+
+ ret = __phy_modify(phydev, RTL8211E_LCR, LCR_MASK(led), lcr_bits);
+
+err:
+ return phy_restore_page(phydev, oldpage, ret);
+}
+
static int rtl8201_ack_interrupt(struct phy_device *phydev)
{
int err;
@@ -330,6 +417,7 @@ static struct phy_driver realtek_drvs[] = {
.config_init = &rtl8211e_config_init,
.ack_interrupt = &rtl821x_ack_interrupt,
.config_intr = &rtl8211e_config_intr,
+ .config_led = &rtl8211e_config_led,
.suspend = genphy_suspend,
.resume = genphy_resume,
.read_page = rtl821x_read_page,
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH v6 3/4] net: phy: realtek: Add helpers for accessing RTL8211x extension pages
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190813191147.19936-1-mka@chromium.org>
Some RTL8211x PHYs have extension pages, which can be accessed
after selecting a page through a custom method. Add a function to
modify bits in a register of an extension page and a helper for
selecting an ext page. Use rtl8211x_modify_ext_paged() in
rtl8211e_config_init() instead of doing things 'manually'.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- none
Changes in v5:
- renamed 'rtl8211e_<action>_ext_page' to 'rtl8211x_<action>_ext_page'
- updated commit message
Changes in v4:
- don't add constant RTL8211E_EXT_PAGE, it's only used once,
use a literal instead
- pass 'oldpage' to phy_restore_page() in rtl8211e_select_ext_page(),
not 'page'
- return 'oldpage' in rtl8211e_select_ext_page()
- use __phy_modify() in rtl8211e_modify_ext_paged() instead of
reimplementing __phy_modify_changed()
- in rtl8211e_modify_ext_paged() return directly when
rtl8211e_select_ext_page() fails
Changes in v3:
- use the new function in rtl8211e_config_init() instead of
doing things 'manually'
- use existing RTL8211E_EXT_PAGE instead of adding a new define
- updated commit message
Changes in v2:
- use phy_select_page() and phy_restore_page(), get rid of
rtl8211e_restore_page()
- s/rtl821e_select_ext_page/rtl8211e_select_ext_page/
- updated commit message
---
drivers/net/phy/realtek.c | 47 +++++++++++++++++++++++++++------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a669945eb829..a5b3708dc4d8 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -53,6 +53,36 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
}
+static int rtl8211x_select_ext_page(struct phy_device *phydev, int page)
+{
+ int ret, oldpage;
+
+ oldpage = phy_select_page(phydev, 7);
+ if (oldpage < 0)
+ return oldpage;
+
+ ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, page);
+ if (ret)
+ return phy_restore_page(phydev, oldpage, ret);
+
+ return oldpage;
+}
+
+static int rtl8211x_modify_ext_paged(struct phy_device *phydev, int page,
+ u32 regnum, u16 mask, u16 set)
+{
+ int ret = 0;
+ int oldpage;
+
+ oldpage = rtl8211x_select_ext_page(phydev, page);
+ if (oldpage < 0)
+ return oldpage;
+
+ ret = __phy_modify(phydev, regnum, mask, set);
+
+ return phy_restore_page(phydev, oldpage, ret);
+}
+
static int rtl8201_ack_interrupt(struct phy_device *phydev)
{
int err;
@@ -184,7 +214,6 @@ static int rtl8211f_config_init(struct phy_device *phydev)
static int rtl8211e_config_init(struct phy_device *phydev)
{
- int ret = 0, oldpage;
u16 val;
/* enable TX/RX delay for rgmii-* modes, and disable them for rgmii. */
@@ -213,19 +242,9 @@ static int rtl8211e_config_init(struct phy_device *phydev)
* 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
* for details).
*/
- oldpage = phy_select_page(phydev, 0x7);
- if (oldpage < 0)
- goto err_restore_page;
-
- ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
- if (ret)
- goto err_restore_page;
-
- ret = __phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
- val);
-
-err_restore_page:
- return phy_restore_page(phydev, oldpage, ret);
+ return rtl8211x_modify_ext_paged(phydev, 0xa4, 0x1c,
+ RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
+ val);
}
static int rtl8211b_suspend(struct phy_device *phydev)
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH v6 2/4] net: phy: Add support for generic LED configuration through the DT
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190813191147.19936-1-mka@chromium.org>
For PHYs with a device tree node look for LED trigger configuration
using the generic binding, if it exists try to apply it via the new
driver hook .config_led.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- delete unnecessary of_node_put() inside for_each_child_of_node()
loop
- use continue instead of goto in of_phy_config_leds()
- check return value of ->config_led() and print a warning if !0
Changes in v5:
- add callback to configure a LED to the PHY driver, instead of
having the driver retrieve the DT data
- use new trigger names
- added support for trigger 'none'
- release DT nodes after use
- renamed 'PHY_LED_LINK_*' to 'PHY_LED_TRIGGER_LINK_*'
- added anonymous struct to struct phy_led_config to track
'activity' in a separate flag. this could be changed to 'flags' if
needed/desired.
- updated commit message (previous subject was 'net: phy: Add
function to retrieve LED configuration from the DT')
Changes in v4:
- patch added to the series
---
drivers/net/phy/phy_device.c | 72 ++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 22 +++++++++++
2 files changed, 94 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6b5cb87f3866..80315777ae67 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -29,6 +29,7 @@
#include <linux/phy_led_triggers.h>
#include <linux/mdio.h>
#include <linux/io.h>
+#include <linux/of.h>
#include <linux/uaccess.h>
MODULE_DESCRIPTION("PHY library");
@@ -1064,6 +1065,75 @@ static int phy_poll_reset(struct phy_device *phydev)
return 0;
}
+static void of_phy_config_leds(struct phy_device *phydev)
+{
+ struct device_node *np, *child;
+ struct phy_led_config cfg;
+ const char *trigger;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_OF_MDIO) || !phydev->drv->config_led)
+ return;
+
+ np = of_find_node_by_name(phydev->mdio.dev.of_node, "leds");
+ if (!np)
+ return;
+
+ for_each_child_of_node(np, child) {
+ u32 led;
+
+ if (of_property_read_u32(child, "reg", &led))
+ continue;
+
+ ret = of_property_read_string(child, "linux,default-trigger",
+ &trigger);
+ if (ret)
+ trigger = "none";
+
+ memset(&cfg, 0, sizeof(cfg));
+
+ if (!strcmp(trigger, "none")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_NONE;
+ } else if (!strcmp(trigger, "phy-link")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK;
+ } else if (!strcmp(trigger, "phy-link-10m")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
+ } else if (!strcmp(trigger, "phy-link-100m")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
+ } else if (!strcmp(trigger, "phy-link-1g")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
+ } else if (!strcmp(trigger, "phy-link-10g")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
+ } else if (!strcmp(trigger, "phy-link-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-10m-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10M;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-100m-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_100M;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-1g-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_1G;
+ cfg.trigger.activity = true;
+ } else if (!strcmp(trigger, "phy-link-10g-activity")) {
+ cfg.trigger.t = PHY_LED_TRIGGER_LINK_10G;
+ cfg.trigger.activity = true;
+ } else {
+ phydev_warn(phydev, "trigger '%s' for LED%d is invalid\n",
+ trigger, led);
+ continue;
+ }
+
+ ret = phydev->drv->config_led(phydev, led, &cfg);
+ if (ret)
+ phydev_warn(phydev, "trigger '%s' for LED%d not supported\n",
+ trigger, led);
+ }
+
+ of_node_put(np);
+}
+
int phy_init_hw(struct phy_device *phydev)
{
int ret = 0;
@@ -1087,6 +1157,8 @@ int phy_init_hw(struct phy_device *phydev)
if (phydev->drv->config_init)
ret = phydev->drv->config_init(phydev);
+ of_phy_config_leds(phydev);
+
return ret;
}
EXPORT_SYMBOL(phy_init_hw);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 462b90b73f93..3a07390fc5e9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -325,6 +325,24 @@ struct phy_c45_device_ids {
u32 device_ids[8];
};
+/* Triggers for PHY LEDs */
+enum phy_led_trigger {
+ PHY_LED_TRIGGER_NONE,
+ PHY_LED_TRIGGER_LINK,
+ PHY_LED_TRIGGER_LINK_10M,
+ PHY_LED_TRIGGER_LINK_100M,
+ PHY_LED_TRIGGER_LINK_1G,
+ PHY_LED_TRIGGER_LINK_10G,
+};
+
+/* Configuration of a single PHY LED */
+struct phy_led_config {
+ struct {
+ enum phy_led_trigger t;
+ bool activity;
+ } trigger;
+};
+
/* phy_device: An instance of a PHY
*
* drv: Pointer to the driver for this PHY instance
@@ -626,6 +644,10 @@ struct phy_driver {
struct ethtool_tunable *tuna,
const void *data);
int (*set_loopback)(struct phy_device *dev, bool enable);
+
+ /* Configure a PHY LED */
+ int (*config_led)(struct phy_device *dev, int led,
+ struct phy_led_config *cfg);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* [PATCH v6 1/4] dt-bindings: net: phy: Add subnode for LED configuration
From: Matthias Kaehlcke @ 2019-08-13 19:11 UTC (permalink / raw)
To: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, Heiner Kallweit
Cc: netdev, devicetree, linux-kernel, Douglas Anderson,
Matthias Kaehlcke
In-Reply-To: <20190813191147.19936-1-mka@chromium.org>
The LED behavior of some Ethernet PHYs is configurable. Add an
optional 'leds' subnode with a child node for each LED to be
configured. The binding aims to be compatible with the common
LED binding (see devicetree/bindings/leds/common.txt).
A LED can be configured to be:
- 'on' when a link is active, some PHYs allow configuration for
certain link speeds
speeds
- 'off'
- blink on RX/TX activity, some PHYs allow configuration for
certain link speeds
For the configuration to be effective it needs to be supported by
the hardware and the corresponding PHY driver.
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v6:
- none
Changes in v5:
- renamed triggers from 'phy_link_<speed>_active' to 'phy-link-<speed>'
- added entries for 'phy-link-<speed>-activity'
- added 'phy-link' and 'phy-link-activity' for triggers with any link
speed
- added entry for trigger 'none'
Changes in v4:
- patch added to the series
---
.../devicetree/bindings/net/ethernet-phy.yaml | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index f70f18ff821f..98ba320f828b 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -153,6 +153,50 @@ properties:
Delay after the reset was deasserted in microseconds. If
this property is missing the delay will be skipped.
+patternProperties:
+ "^leds$":
+ type: object
+ description:
+ Subnode with configuration of the PHY LEDs.
+
+ patternProperties:
+ "^led@[0-9]+$":
+ type: object
+ description:
+ Subnode with the configuration of a single PHY LED.
+
+ properties:
+ reg:
+ description:
+ The ID number of the LED, typically corresponds to a hardware ID.
+ $ref: "/schemas/types.yaml#/definitions/uint32"
+
+ linux,default-trigger:
+ description:
+ This parameter, if present, is a string specifying the trigger
+ assigned to the LED. Supported triggers are:
+ "none" - LED will be solid off
+ "phy-link" - LED will be solid on when a link is active
+ "phy-link-10m" - LED will be solid on when a 10Mb/s link is active
+ "phy-link-100m" - LED will be solid on when a 100Mb/s link is active
+ "phy-link-1g" - LED will be solid on when a 1Gb/s link is active
+ "phy-link-10g" - LED will be solid on when a 10Gb/s link is active
+ "phy-link-activity" - LED will be on when link is active and blink
+ off with activity.
+ "phy-link-10m-activity" - LED will be on when 10Mb/s link is active
+ and blink off with activity.
+ "phy-link-100m-activity" - LED will be on when 100Mb/s link is
+ active and blink off with activity.
+ "phy-link-1g-activity" - LED will be on when 1Gb/s link is active
+ and blink off with activity.
+ "phy-link-10g-activity" - LED will be on when 10Gb/s link is active
+ and blink off with activity.
+
+ $ref: "/schemas/types.yaml#/definitions/string"
+
+ required:
+ - reg
+
required:
- reg
@@ -173,5 +217,20 @@ examples:
reset-gpios = <&gpio1 4 1>;
reset-assert-us = <1000>;
reset-deassert-us = <2000>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ linux,default-trigger = "phy-link-1g";
+ };
+
+ led@1 {
+ reg = <1>;
+ linux,default-trigger = "phy-link-100m-activity";
+ };
+ };
};
};
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox