asynchronous data transport for ethernet driver and lwIP #33
Conversation
| * the GNU General Public License version 2. Note that NO WARRANTY is provided. | ||
| * See "LICENSE_GPLv2.txt" for details. | ||
| * | ||
| * @TAG(DATA61_GPL) |
There was a problem hiding this comment.
this is no longer used, change to
| * @TAG(DATA61_GPL) | |
| * * SPDX-License-Identifier: GPL-2.0-only |
I wonder, should this become GPL or BSD?
There was a problem hiding this comment.
You're right, by default it should be BSD unless there is some specific reason for GPL (i.e. a GPL dependency or otherwise derived from GPL code).
The rest of the copyright header should also follow the new shorter line, and unless this code was written at CSIRO already (more than a year ago), it should probably be copyright UNSW.
There was a problem hiding this comment.
This component is GPL due to licensing requirements inherited from using GPL ethernet drivers from U-Boot.
There was a problem hiding this comment.
So this contains code that is copied from U-Boot? In that case, the original U-Boot copyright headers should be here and/or a comment where exactly the code is from. Ot is this linked together with some U-Boot code?
There was a problem hiding this comment.
The header file doesn't contain code from U-Boot, but the Ethdriver component does which makes the whole component a derived work.
There was a problem hiding this comment.
I still don't see any mention of U-Boot anywhere in the repo apart from TimeServer. Am I missing something?
|
|
||
| struct netif *netif = netif_find("e0"); | ||
| if (netif == NULL) { | ||
| ZF_LOGE("No device registered to call dhcp on or start"); |
|
|
||
| bool timers_initialised; | ||
|
|
||
| uint64_t (*timer_get_time_fn)(void); |
There was a problem hiding this comment.
consider defining the function type in some header file
typedef uint64_t (*timer_get_time_fn_t)(void);
| connection LwipEthernetAsyncServerInit name##_server_init(from driver.name##_init1, to driver.name##_init2); | ||
|
|
||
| #define lwip_ethernet_async_configurations(name, client, driver) \ | ||
| name##_dma.size = 0x400000; \ |
There was a problem hiding this comment.
please add a comment if the numbers are randomly chosen or is they have to follow certain rules.
| import <VirtQueue/VirtQueue.camkes>; | ||
|
|
||
| procedure lwip_ethernet_async_control { | ||
| void mac(out uint8_t b1, out uint8_t b2, out uint8_t b3, out uint8_t b4, out uint8_t b5, out uint8_t b6); |
There was a problem hiding this comment.
Parameter handling get much simpler if we just use an uint64_t for the MAC and define this to use network byte order. Compilers can handle u64 and bit-masking even on 32-bit systems nicely.
| * @param cookie client state data. | ||
| * | ||
| */ | ||
| static void rx_queue(void *cookie) |
There was a problem hiding this comment.
Could make the name a bit more self-explaining:
| static void rx_queue(void *cookie) | |
| static void process_rx_queue(void *cookie) |
| int error = reg_rx(rx_queue, data); | ||
| if (error) { | ||
| ZF_LOGE("Unable to register handler"); | ||
| } |
| #include <sel4/sel4.h> | ||
| #include <platsupport/io.h> | ||
|
|
||
| #define ENCODE_DMA_ADDRESS(buf) ({ \ |
There was a problem hiding this comment.
does this really have to be a macro or could it also become a static-inline function?
| uint64_t new_buf = (((uint64_t)wrapped_ptr.id << 32) | ((uint64_t)wrapped_ptr.offset)); \ | ||
| new_buf; }) | ||
|
|
||
| #define DECODE_DMA_ADDRESS(buf) ({\ |
There was a problem hiding this comment.
does this really have to be a macro or could it also become a static-inline function?
| * | ||
| * @return the physical memory address of the buffer. | ||
| */ | ||
| static uintptr_t eth_allocate_rx_buf(void *iface, size_t buf_size, void **cookie) |
There was a problem hiding this comment.
consider defining a dedicated type like phys_addr_t for physical addresses. If these are just normal buffers in the end, it could also be a simple void*
| * @param lens an array on length num_bufs containing the length of data each buffer now contains. | ||
| * | ||
| */ | ||
| static void eth_rx_complete(void *iface, unsigned int num_bufs, void **cookies, unsigned int *lens) |
Hi @lucypa, are there any changes in the pipeline to https://github.com/sel4/camkes for adding example applications that would use these new components? |
Signed-off-by: Lucy <[email protected]>
Signed-off-by: Lucy <[email protected]>
Signed-off-by: Lucy <[email protected]>
Signed-off-by: Lucy <[email protected]>
Signed-off-by: Lucy <[email protected]>
|
Hi @kent-mcleod |
| buff_desc_t *desc = cookies[i]; | ||
| int err = enqueue_used(state->rx_ring, desc->encoded_addr, (size_t)lens[i], desc->cookie); | ||
| if (err) { | ||
| ZF_LOGE(err, "Enqueue failed: the RX used ring is full"); |
There was a problem hiding this comment.
This ZF_LOGE should probably be changed to ZF_LOGE_IFERR or the first parameter needs to be a string if the error isn't conditional
| ZF_LOGE(err, "Enqueue failed: the RX used ring is full"); | |
| ZF_LOGE("Enqueue failed: the RX used ring is full"); |
These files create an asynchronous communication layer between a driver and the lwIP network stack using seL4/projects_libs#15
This is part of the improvement to the seL4 device driver framework.
Test with: seL4/projects_libs#15