[PATCH] rtnet: Fix lifecycle management of mapped rtskbs

Philippe Gerum rpm at xenomai.org
Fri Jan 18 12:58:43 CET 2019


On 1/17/19 1:56 PM, Philippe Gerum wrote:
> On 12/18/18 7:46 AM, Jan Kiszka via Xenomai wrote:
>> On 14.12.18 12:22, Jan Kiszka via Xenomai wrote:
>>> Do not add rtskbs to the rtskb_list which are not mappend because
>>> rtdev_unmap_rtskb will not remove such rtskbs again (buf_dma_addr ==
>>> RTSKB_UNMAPPED). In fact, rtskb_list should be called rtskb_mapped_list,
>>> so refactor this while at it.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
>>> ---
>>>   kernel/drivers/net/stack/rtdev.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/drivers/net/stack/rtdev.c
>>> b/kernel/drivers/net/stack/rtdev.c
>>> index 5eb73ce1a4..7b267f2a7c 100644
>>> --- a/kernel/drivers/net/stack/rtdev.c
>>> +++ b/kernel/drivers/net/stack/rtdev.c
>>> @@ -44,9 +44,9 @@ MODULE_PARM_DESC(device_rtskbs, "Number of
>>> additional global realtime socket "
>>>   struct rtnet_device         *rtnet_devices[MAX_RT_DEVICES];
>>>   static struct rtnet_device  *loopback_device;
>>>   static DEFINE_RTDM_LOCK(rtnet_devices_rt_lock);
>>> +static LIST_HEAD(rtskb_mapped_list);
>>>     LIST_HEAD(event_hook_list);
>>> -LIST_HEAD(rtskb_list);
>>>   DEFINE_MUTEX(rtnet_devices_nrt_lock);
>>>     static int rtdev_locked_xmit(struct rtskb *skb, struct
>>> rtnet_device *rtdev);
>>> @@ -412,8 +412,8 @@ int rtdev_map_rtskb(struct rtskb *skb)
>>>       }
>>>       }
>>>   -    if (!err)
>>> -    list_add(&skb->entry, &rtskb_list);
>>> +    if (!err && skb->buf_dma_addr != RTSKB_UNMAPPED)
>>> +    list_add(&skb->entry, &rtskb_mapped_list);
>>>         mutex_unlock(&rtnet_devices_nrt_lock);
>>>   @@ -430,7 +430,7 @@ static int rtdev_map_all_rtskbs(struct
>>> rtnet_device *rtdev)
>>>       if (!rtdev->map_rtskb)
>>>       return 0;
>>>   -    list_for_each_entry(skb, &rtskb_list, entry) {
>>> +    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>>>       err = rtskb_map(rtdev, skb);
>>>       if (err)
>>>          break;
>>> @@ -474,7 +474,7 @@ static void rtdev_unmap_all_rtskbs(struct
>>> rtnet_device *rtdev)
>>>       if (!rtdev->unmap_rtskb)
>>>       return;
>>>   -    list_for_each_entry(skb, &rtskb_list, entry) {
>>> +    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>>>       rtdev->unmap_rtskb(rtdev, skb);
>>>       }
>>>   }
>>>
>>
>> Applied to master and stable.
>>
>> Jan
>>
> 
> Unfortunately, this breaks RTnet for any driver using DMA, apparently
> uncovering a pre-existing issue in the device init->registration logic
> wrt mapping skb from per-device pools.
> 
> In short, filtering skbs we can't immediately map out of the rtskb list
> in rtdev_map_rtskb() prevents rt_register_rtnetdev(dev) from finding
> @dev's skbs in rtskb_mapped_list when running rtdev_map_all_skbs().
> In turn this prevents the driver from setting up the hw's RX/TX rings
> with proper DMA mapping addresses, as none of its skbs has valid bus
> addresses for DMA.
> 
> There may be a simple fix, but the rtdev init logic is a bit too
> convoluted for my after lunch. I'll try to come up with a fix later.
> 

So there is a catch 22 situation with skb mapping for DMA operations. It
stems from the fact that creating a netdevice via a call to
rt_alloc_etherdev(dev) can't successfully map the skbs attached to that
new device, because rtdev_map_rtskb() only operates on registered
netdevices, which the new one cannot be just yet as it is only half-baked.

Prior to that change, the code worked almost by accident because
registering that new device would end up calling rtdev_map_all_rtskbs(),
which would eventually find the unmapped skbs left in the global
rtskb_list and map them through the driver's ->map_rtskb handler.

There are only two in-tree drivers using the ->map_rtskb handler, namely
igb and e1000e, I'm working on adding a 3rd one with a refactoring of
the fec for i.MX SoCs, which is why I hit this bug.

I suspect ->map_rtskb was introduced to address yet another issue with
all other DMA-capable drivers: calling dma_{map, unmap}_single() from
I/O handlers in primary mode like they all do is unsafe, and this
handler may have been intended to ensure those operations happen from
regular Linux context instead. As a matter of fact, most existing RTnet
drivers are unsafe regarding this. Generally speaking, it looks like the
process of initializing a new netdevice has evolved over time in RTnet,
leading to a lot of open coding on the driver developer's end, which is
quite error-prone.

My take on this is that the init->registration logic may need a rework
and a better API, solving the issue of skb mapping in the same move,
fixing or simply dropping obsolete driver code too. However, doing so
without breaking more stuff would require significant RTnet background I
don't have, so I'm leaving this to the grown-ups.

In the meantime, this is a workaround addressing the skb mapping bug
introduced by this commit (only lightly tested so far):

commit 31ae7b2154bc0d8df841835d699087876bec3014
Author: Philippe Gerum <rpm at xenomai.org>
Date:   Thu Jan 17 18:17:43 2019 +0100

    net/rtdev: ensure per-device skbs get mapped at registration

    This patch works around a regression introduced by #74464ee37d0,
    causing a new device's skbs not to be passed to its ->map_rtskb()
    handler when registered, breaking further DMA inits in the driver.

diff --git a/kernel/drivers/net/stack/rtdev.c
b/kernel/drivers/net/stack/rtdev.c
index 2982740d0..c5f8c73c6 100644
--- a/kernel/drivers/net/stack/rtdev.c
+++ b/kernel/drivers/net/stack/rtdev.c
@@ -45,6 +45,7 @@ struct rtnet_device
*rtnet_devices[MAX_RT_DEVICES];
 static struct rtnet_device  *loopback_device;
 static DEFINE_RTDM_LOCK(rtnet_devices_rt_lock);
 static LIST_HEAD(rtskb_mapped_list);
+static LIST_HEAD(rtskb_mapwait_list);
  LIST_HEAD(event_hook_list);
 DEFINE_MUTEX(rtnet_devices_nrt_lock);
@@ -459,8 +460,12 @@ int rtdev_map_rtskb(struct rtskb *skb)
 	}
     }
 -    if (!err && skb->buf_dma_addr != RTSKB_UNMAPPED)
-	list_add(&skb->entry, &rtskb_mapped_list);
+    if (!err) {
+	    if (skb->buf_dma_addr != RTSKB_UNMAPPED)
+		    list_add(&skb->entry, &rtskb_mapped_list);
+	    else
+		    list_add(&skb->entry, &rtskb_mapwait_list);
+    }
      mutex_unlock(&rtnet_devices_nrt_lock);
 @@ -471,19 +476,31 @@ int rtdev_map_rtskb(struct rtskb *skb)
  static int rtdev_map_all_rtskbs(struct rtnet_device *rtdev)
 {
-    struct rtskb *skb;
-    int err = 0;
+    struct rtskb *skb, *n;
+    int err;
      if (!rtdev->map_rtskb)
-	return 0;
+	    return 0;
 -    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
-	err = rtskb_map(rtdev, skb);
-	if (err)
-	   break;
+    if (!list_empty(&rtskb_mapped_list)) {
+	    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
+		    err = rtskb_map(rtdev, skb);
+		    if (err)
+			    return err;
+	    }
     }
 -    return err;
+    if (!list_empty(&rtskb_mapwait_list)) {
+	    list_for_each_entry_safe(skb, n, &rtskb_mapwait_list, entry) {
+		    err = rtskb_map(rtdev, skb);
+		    if (err)
+			    return err;
+		    list_del(&skb->entry);
+		    list_add(&skb->entry, &rtskb_mapped_list);
+	    }
+    }
+
+    return 0;
 }


-- 
Philippe.



More information about the Xenomai mailing list