* [PATCH net v8 0/2] net/ps3_gelic_net: DMA related fixes
@ 2023-03-11 21:46 Geoff Levand
2023-03-11 21:46 ` [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand
2023-03-11 21:46 ` [PATCH net v8 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand
0 siblings, 2 replies; 7+ messages in thread
From: Geoff Levand @ 2023-03-11 21:46 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Alexander Lobakin, Alexander Duyck, Paolo Abeni
v8: Add more cpu_to_be32 calls.
v7: Remove all cleanups, sync to spider net.
v6: Reworked and cleaned up patches.
v5: Some additional patch cleanups.
v4: More patch cleanups.
v3: Cleaned up patches as requested.
Geoff Levand (2):
net/ps3_gelic_net: Fix RX sk_buff length
net/ps3_gelic_net: Use dma_mapping_error
drivers/net/ethernet/toshiba/ps3_gelic_net.c | 43 +++++++++++---------
drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 ++-
2 files changed, 27 insertions(+), 21 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length
2023-03-11 21:46 [PATCH net v8 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand
@ 2023-03-11 21:46 ` Geoff Levand
2023-03-11 22:37 ` Alexander H Duyck
2023-03-11 21:46 ` [PATCH net v8 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand
1 sibling, 1 reply; 7+ messages in thread
From: Geoff Levand @ 2023-03-11 21:46 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Alexander Lobakin, Alexander Duyck, Paolo Abeni
The Gelic Ethernet device needs to have the RX sk_buffs aligned to
GELIC_NET_RXBUF_ALIGN, and also the length of the RX sk_buffs must
be a multiple of GELIC_NET_RXBUF_ALIGN.
The current Gelic Ethernet driver was not allocating sk_buffs large
enough to allow for this alignment.
Also, correct the maximum and minimum MTU sizes, and add a new
preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME.
Fixes various randomly occurring runtime network errors.
Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
Signed-off-by: Geoff Levand <geoff@infradead•org>
---
drivers/net/ethernet/toshiba/ps3_gelic_net.c | 21 +++++++++++---------
drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 +++--
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index cf8de8a7a8a1..56557fc8d18a 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -44,6 +44,14 @@ MODULE_AUTHOR("SCE Inc.");
MODULE_DESCRIPTION("Gelic Network driver");
MODULE_LICENSE("GPL");
+/**
+ * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
+ * must be a multiple of GELIC_NET_RXBUF_ALIGN.
+ */
+
+static const unsigned int gelic_rx_skb_size =
+ ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
+ GELIC_NET_RXBUF_ALIGN - 1;
/* set irq_mask */
int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
@@ -370,21 +378,16 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
struct gelic_descr *descr)
{
int offset;
- unsigned int bufsize;
if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
dev_info(ctodev(card), "%s: ERROR status\n", __func__);
- /* we need to round up the buffer size to a multiple of 128 */
- bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
- /* and we need to have it 128 byte aligned, therefore we allocate a
- * bit more */
- descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
+ descr->skb = netdev_alloc_skb(*card->netdev, gelic_rx_skb_size);
if (!descr->skb) {
descr->buf_addr = 0; /* tell DMAC don't touch memory */
return -ENOMEM;
}
- descr->buf_size = cpu_to_be32(bufsize);
+ descr->buf_size = cpu_to_be32(gelic_rx_skb_size);
descr->dmac_cmd_status = 0;
descr->result_size = 0;
descr->valid_size = 0;
@@ -397,7 +400,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
/* io-mmu-map the skb */
descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
descr->skb->data,
- GELIC_NET_MAX_MTU,
+ gelic_rx_skb_size,
DMA_FROM_DEVICE));
if (!descr->buf_addr) {
dev_kfree_skb_any(descr->skb);
@@ -915,7 +918,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
data_error = be32_to_cpu(descr->data_error);
/* unmap skb buffer */
dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
- GELIC_NET_MAX_MTU,
+ gelic_rx_skb_size,
DMA_FROM_DEVICE);
skb_put(skb, be32_to_cpu(descr->valid_size)?
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
index 68f324ed4eaf..0d98defb011e 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
@@ -19,8 +19,9 @@
#define GELIC_NET_RX_DESCRIPTORS 128 /* num of descriptors */
#define GELIC_NET_TX_DESCRIPTORS 128 /* num of descriptors */
-#define GELIC_NET_MAX_MTU VLAN_ETH_FRAME_LEN
-#define GELIC_NET_MIN_MTU VLAN_ETH_ZLEN
+#define GELIC_NET_MAX_FRAME 2312
+#define GELIC_NET_MAX_MTU 2294
+#define GELIC_NET_MIN_MTU 64
#define GELIC_NET_RXBUF_ALIGN 128
#define GELIC_CARD_RX_CSUM_DEFAULT 1 /* hw chksum */
#define GELIC_NET_WATCHDOG_TIMEOUT 5*HZ
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net v8 2/2] net/ps3_gelic_net: Use dma_mapping_error
2023-03-11 21:46 [PATCH net v8 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand
2023-03-11 21:46 ` [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand
@ 2023-03-11 21:46 ` Geoff Levand
2023-03-11 22:41 ` Alexander H Duyck
1 sibling, 1 reply; 7+ messages in thread
From: Geoff Levand @ 2023-03-11 21:46 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller
Cc: Alexander Lobakin, Alexander Duyck, Paolo Abeni
The current Gelic Etherenet driver was checking the return value of its
dma_map_single call, and not using the dma_mapping_error() routine.
Fixes runtime problems like these:
DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
Signed-off-by: Geoff Levand <geoff@infradead•org>
---
drivers/net/ethernet/toshiba/ps3_gelic_net.c | 24 +++++++++++---------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index 56557fc8d18a..87d3c768286e 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -325,15 +325,17 @@ static int gelic_card_init_chain(struct gelic_card *card,
/* set up the hardware pointers in each descriptor */
for (i = 0; i < no; i++, descr++) {
+ dma_addr_t cpu_addr;
+
gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
- descr->bus_addr =
- dma_map_single(ctodev(card), descr,
- GELIC_DESCR_SIZE,
- DMA_BIDIRECTIONAL);
- if (!descr->bus_addr)
+ cpu_addr = dma_map_single(ctodev(card), descr,
+ GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
+
+ if (dma_mapping_error(ctodev(card), cpu_addr))
goto iommu_error;
+ descr->bus_addr = cpu_to_be32(cpu_addr);
descr->next = descr + 1;
descr->prev = descr - 1;
}
@@ -377,6 +379,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
static int gelic_descr_prepare_rx(struct gelic_card *card,
struct gelic_descr *descr)
{
+ dma_addr_t cpu_addr;
int offset;
if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
@@ -398,11 +401,10 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
if (offset)
skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
/* io-mmu-map the skb */
- descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
- descr->skb->data,
- gelic_rx_skb_size,
- DMA_FROM_DEVICE));
- if (!descr->buf_addr) {
+ cpu_addr = dma_map_single(ctodev(card), descr->skb->data,
+ gelic_rx_skb_size, DMA_FROM_DEVICE);
+ descr->buf_addr = cpu_to_be32(cpu_addr);
+ if (dma_mapping_error(ctodev(card), cpu_addr)) {
dev_kfree_skb_any(descr->skb);
descr->skb = NULL;
dev_info(ctodev(card),
@@ -782,7 +784,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
- if (!buf) {
+ if (dma_mapping_error(ctodev(card), buf)) {
dev_err(ctodev(card),
"dma map 2 failed (%p, %i). Dropping packet\n",
skb->data, skb->len);
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length
2023-03-11 21:46 ` [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand
@ 2023-03-11 22:37 ` Alexander H Duyck
2023-03-18 17:37 ` Geoff Levand
0 siblings, 1 reply; 7+ messages in thread
From: Alexander H Duyck @ 2023-03-11 22:37 UTC (permalink / raw)
To: Geoff Levand, netdev, Jakub Kicinski, David S. Miller
Cc: Alexander Lobakin, Paolo Abeni
On Sat, 2023-03-11 at 21:46 +0000, Geoff Levand wrote:
> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
> GELIC_NET_RXBUF_ALIGN, and also the length of the RX sk_buffs must
> be a multiple of GELIC_NET_RXBUF_ALIGN.
>
> The current Gelic Ethernet driver was not allocating sk_buffs large
> enough to allow for this alignment.
>
> Also, correct the maximum and minimum MTU sizes, and add a new
> preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME.
>
> Fixes various randomly occurring runtime network errors.
>
> Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
> Signed-off-by: Geoff Levand <geoff@infradead•org>
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 21 +++++++++++---------
> drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 +++--
> 2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cf8de8a7a8a1..56557fc8d18a 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -44,6 +44,14 @@ MODULE_AUTHOR("SCE Inc.");
> MODULE_DESCRIPTION("Gelic Network driver");
> MODULE_LICENSE("GPL");
>
> +/**
> + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
> + * must be a multiple of GELIC_NET_RXBUF_ALIGN.
> + */
> +
> +static const unsigned int gelic_rx_skb_size =
> + ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
> + GELIC_NET_RXBUF_ALIGN - 1;
>
After a bit more digging I now understand the need for the
"GELIC_NET_RXBUF_ALIGN - 1". It shouldn't be added here. The device
will not be able to DMA into it. It is being used to align the buffer
itself to 128B. I am assuming it must be 128B aligned in BOTH size and
offset.
> /* set irq_mask */
> int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
> @@ -370,21 +378,16 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> int offset;
> - unsigned int bufsize;
>
> if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
> dev_info(ctodev(card), "%s: ERROR status\n", __func__);
> - /* we need to round up the buffer size to a multiple of 128 */
> - bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
>
> - /* and we need to have it 128 byte aligned, therefore we allocate a
> - * bit more */
> - descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
> + descr->skb = netdev_alloc_skb(*card->netdev, gelic_rx_skb_size);
I would leave the "+ GELIC_NET_RXBUF_ALIGN - 1" here. so it should be
descr->skb = netdev_alloc_skb(*card->netdev,
gelic_rx_skb_size +
GELIC_NET_RXBUF_ALIGN - 1);
Also I would leav the comment as it makes it a bit clearer what is
going on here.
> if (!descr->skb) {
> descr->buf_addr = 0; /* tell DMAC don't touch memory */
> return -ENOMEM;
> }
> - descr->buf_size = cpu_to_be32(bufsize);
> + descr->buf_size = cpu_to_be32(gelic_rx_skb_size);
> descr->dmac_cmd_status = 0;
> descr->result_size = 0;
> descr->valid_size = 0;
The part I missed was the lines of code that didn't make it into the
patch that like between the two code blocks here above and below. They
are doing an AND with your align mask and then adding the difference to
the skb reserve to pad it to be 128B aligned.
> @@ -397,7 +400,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
> /* io-mmu-map the skb */
> descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> descr->skb->data,
> - GELIC_NET_MAX_MTU,
> + gelic_rx_skb_size,
> DMA_FROM_DEVICE));
> if (!descr->buf_addr) {
> dev_kfree_skb_any(descr->skb);
> @@ -915,7 +918,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
> data_error = be32_to_cpu(descr->data_error);
> /* unmap skb buffer */
> dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
> - GELIC_NET_MAX_MTU,
> + gelic_rx_skb_size,
> DMA_FROM_DEVICE);
>
> skb_put(skb, be32_to_cpu(descr->valid_size)?
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> index 68f324ed4eaf..0d98defb011e 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> @@ -19,8 +19,9 @@
> #define GELIC_NET_RX_DESCRIPTORS 128 /* num of descriptors */
> #define GELIC_NET_TX_DESCRIPTORS 128 /* num of descriptors */
>
> -#define GELIC_NET_MAX_MTU VLAN_ETH_FRAME_LEN
> -#define GELIC_NET_MIN_MTU VLAN_ETH_ZLEN
> +#define GELIC_NET_MAX_FRAME 2312
> +#define GELIC_NET_MAX_MTU 2294
> +#define GELIC_NET_MIN_MTU 64
> #define GELIC_NET_RXBUF_ALIGN 128
> #define GELIC_CARD_RX_CSUM_DEFAULT 1 /* hw chksum */
> #define GELIC_NET_WATCHDOG_TIMEOUT 5*HZ
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v8 2/2] net/ps3_gelic_net: Use dma_mapping_error
2023-03-11 21:46 ` [PATCH net v8 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand
@ 2023-03-11 22:41 ` Alexander H Duyck
2023-03-18 17:37 ` Geoff Levand
0 siblings, 1 reply; 7+ messages in thread
From: Alexander H Duyck @ 2023-03-11 22:41 UTC (permalink / raw)
To: Geoff Levand, netdev, Jakub Kicinski, David S. Miller
Cc: Alexander Lobakin, Paolo Abeni
On Sat, 2023-03-11 at 21:46 +0000, Geoff Levand wrote:
> The current Gelic Etherenet driver was checking the return value of its
> dma_map_single call, and not using the dma_mapping_error() routine.
>
> Fixes runtime problems like these:
>
> DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
> WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>
> Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
> Signed-off-by: Geoff Levand <geoff@infradead•org>
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 24 +++++++++++---------
> 1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 56557fc8d18a..87d3c768286e 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -325,15 +325,17 @@ static int gelic_card_init_chain(struct gelic_card *card,
>
> /* set up the hardware pointers in each descriptor */
> for (i = 0; i < no; i++, descr++) {
> + dma_addr_t cpu_addr;
> +
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> - descr->bus_addr =
> - dma_map_single(ctodev(card), descr,
> - GELIC_DESCR_SIZE,
> - DMA_BIDIRECTIONAL);
>
> - if (!descr->bus_addr)
> + cpu_addr = dma_map_single(ctodev(card), descr,
> + GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
> +
> + if (dma_mapping_error(ctodev(card), cpu_addr))
> goto iommu_error;
>
> + descr->bus_addr = cpu_to_be32(cpu_addr);
> descr->next = descr + 1;
> descr->prev = descr - 1;
> }
> @@ -377,6 +379,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
> static int gelic_descr_prepare_rx(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> + dma_addr_t cpu_addr;
> int offset;
>
> if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
> @@ -398,11 +401,10 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
> if (offset)
> skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> /* io-mmu-map the skb */
> - descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> - descr->skb->data,
> - gelic_rx_skb_size,
> - DMA_FROM_DEVICE));
> - if (!descr->buf_addr) {
> + cpu_addr = dma_map_single(ctodev(card), descr->skb->data,
> + gelic_rx_skb_size, DMA_FROM_DEVICE);
> + descr->buf_addr = cpu_to_be32(cpu_addr);
> + if (dma_mapping_error(ctodev(card), cpu_addr)) {
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> dev_info(ctodev(card),
> @@ -782,7 +784,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
>
> buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
>
> - if (!buf) {
> + if (dma_mapping_error(ctodev(card), buf)) {
> dev_err(ctodev(card),
> "dma map 2 failed (%p, %i). Dropping packet\n",
> skb->data, skb->len);
Looks good to me. The only tweak I would make is maybe using "dma_addr"
, "phys_addr" or "bus_addr" instead of "cpu_addr", however that is more
cosmetic than anything else.
Reviewed-by: Alexander Duyck <alexanderduyck@fb•com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length
2023-03-11 22:37 ` Alexander H Duyck
@ 2023-03-18 17:37 ` Geoff Levand
0 siblings, 0 replies; 7+ messages in thread
From: Geoff Levand @ 2023-03-18 17:37 UTC (permalink / raw)
To: Alexander H Duyck, netdev, Jakub Kicinski, David S. Miller
Cc: Alexander Lobakin, Paolo Abeni
Hi,
Sorry for the delay in my reply, I was away last weekend and
couldn't do any PS3 work.
On 3/11/23 14:37, Alexander H Duyck wrote:
> On Sat, 2023-03-11 at 21:46 +0000, Geoff Levand wrote:
>> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
>> GELIC_NET_RXBUF_ALIGN, and also the length of the RX sk_buffs must
>> be a multiple of GELIC_NET_RXBUF_ALIGN.
>>
>> The current Gelic Ethernet driver was not allocating sk_buffs large
>> enough to allow for this alignment.
>>
>> Also, correct the maximum and minimum MTU sizes, and add a new
>> preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME.
>>
>> Fixes various randomly occurring runtime network errors.
>>
>> Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
>> Signed-off-by: Geoff Levand <geoff@infradead•org>
>> ---
>> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 21 +++++++++++---------
>> drivers/net/ethernet/toshiba/ps3_gelic_net.h | 5 +++--
>> 2 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index cf8de8a7a8a1..56557fc8d18a 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -44,6 +44,14 @@ MODULE_AUTHOR("SCE Inc.");
>> MODULE_DESCRIPTION("Gelic Network driver");
>> MODULE_LICENSE("GPL");
>>
>> +/**
>> + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
>> + * must be a multiple of GELIC_NET_RXBUF_ALIGN.
>> + */
>> +
>> +static const unsigned int gelic_rx_skb_size =
>> + ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
>> + GELIC_NET_RXBUF_ALIGN - 1;
>>
>
> After a bit more digging I now understand the need for the
> "GELIC_NET_RXBUF_ALIGN - 1". It shouldn't be added here. The device
> will not be able to DMA into it. It is being used to align the buffer
> itself to 128B. I am assuming it must be 128B aligned in BOTH size and
> offset.
I want gelic_rx_skb_size to be the value to use with netdev_alloc_skb.
Also, I think just using GELIC_NET_MAX_FRAME for dma_map_single is
enough. Using gelic_rx_skb_size for dma_map_single would work, but the
gelic hardware device could only fill GELIC_NET_MAX_FRAME of the
bigger gelic_rx_skb_size DMA mapping.
>> /* set irq_mask */
>> int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
>> @@ -370,21 +378,16 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>> struct gelic_descr *descr)
>> {
>> int offset;
>> - unsigned int bufsize;
>>
>> if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
>> dev_info(ctodev(card), "%s: ERROR status\n", __func__);
>> - /* we need to round up the buffer size to a multiple of 128 */
>> - bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
>>
>> - /* and we need to have it 128 byte aligned, therefore we allocate a
>> - * bit more */
>> - descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
>> + descr->skb = netdev_alloc_skb(*card->netdev, gelic_rx_skb_size);
>
> I would leave the "+ GELIC_NET_RXBUF_ALIGN - 1" here. so it should be
> descr->skb = netdev_alloc_skb(*card->netdev,
> gelic_rx_skb_size +
> GELIC_NET_RXBUF_ALIGN - 1);
>
> Also I would leav the comment as it makes it a bit clearer what is
> going on here.
With the use of GELIC_NET_MAX_FRAME as the DMA mapping size
gelic_rx_skb_size becomes local to the gelic_descr_prepare_rx
routine, so I put the comment about the size requirements into
the the comment for the gelic_descr_prepare_rx routine.
>
>> if (!descr->skb) {
>> descr->buf_addr = 0; /* tell DMAC don't touch memory */
>> return -ENOMEM;
>> }
>> - descr->buf_size = cpu_to_be32(bufsize);
>> + descr->buf_size = cpu_to_be32(gelic_rx_skb_size);
>> descr->dmac_cmd_status = 0;
>> descr->result_size = 0;
>> descr->valid_size = 0;
>
> The part I missed was the lines of code that didn't make it into the
> patch that like between the two code blocks here above and below. They
> are doing an AND with your align mask and then adding the difference to
> the skb reserve to pad it to be 128B aligned.
>
>> @@ -397,7 +400,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>> /* io-mmu-map the skb */
>> descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
>> descr->skb->data,
>> - GELIC_NET_MAX_MTU,
>> + gelic_rx_skb_size,
>> DMA_FROM_DEVICE));
>> if (!descr->buf_addr) {
>> dev_kfree_skb_any(descr->skb);
>> @@ -915,7 +918,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
>> data_error = be32_to_cpu(descr->data_error);
>> /* unmap skb buffer */
>> dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
>> - GELIC_NET_MAX_MTU,
>> + gelic_rx_skb_size,
>> DMA_FROM_DEVICE);
>>
>> skb_put(skb, be32_to_cpu(descr->valid_size)?
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
>> index 68f324ed4eaf..0d98defb011e 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
>> @@ -19,8 +19,9 @@
>> #define GELIC_NET_RX_DESCRIPTORS 128 /* num of descriptors */
>> #define GELIC_NET_TX_DESCRIPTORS 128 /* num of descriptors */
>>
>> -#define GELIC_NET_MAX_MTU VLAN_ETH_FRAME_LEN
>> -#define GELIC_NET_MIN_MTU VLAN_ETH_ZLEN
>> +#define GELIC_NET_MAX_FRAME 2312
>> +#define GELIC_NET_MAX_MTU 2294
>> +#define GELIC_NET_MIN_MTU 64
>> #define GELIC_NET_RXBUF_ALIGN 128
>> #define GELIC_CARD_RX_CSUM_DEFAULT 1 /* hw chksum */
>> #define GELIC_NET_WATCHDOG_TIMEOUT 5*HZ
-Geoff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v8 2/2] net/ps3_gelic_net: Use dma_mapping_error
2023-03-11 22:41 ` Alexander H Duyck
@ 2023-03-18 17:37 ` Geoff Levand
0 siblings, 0 replies; 7+ messages in thread
From: Geoff Levand @ 2023-03-18 17:37 UTC (permalink / raw)
To: Alexander H Duyck, netdev, Jakub Kicinski, David S. Miller
Cc: Alexander Lobakin, Paolo Abeni
On 3/11/23 14:41, Alexander H Duyck wrote:
> On Sat, 2023-03-11 at 21:46 +0000, Geoff Levand wrote:
>> The current Gelic Etherenet driver was checking the return value of its
>> dma_map_single call, and not using the dma_mapping_error() routine.
>>
>> Fixes runtime problems like these:
>>
>> DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
>> WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>>
>> Fixes: 02c1889166b4 ("ps3: gigabit ethernet driver for PS3, take3")
>> Signed-off-by: Geoff Levand <geoff@infradead•org>
>> ---
>> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 24 +++++++++++---------
>> 1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index 56557fc8d18a..87d3c768286e 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -325,15 +325,17 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>
>> /* set up the hardware pointers in each descriptor */
>> for (i = 0; i < no; i++, descr++) {
>> + dma_addr_t cpu_addr;
>> +
>> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>> - descr->bus_addr =
>> - dma_map_single(ctodev(card), descr,
>> - GELIC_DESCR_SIZE,
>> - DMA_BIDIRECTIONAL);
>>
>> - if (!descr->bus_addr)
>> + cpu_addr = dma_map_single(ctodev(card), descr,
>> + GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
>> +
>> + if (dma_mapping_error(ctodev(card), cpu_addr))
>> goto iommu_error;
>>
>> + descr->bus_addr = cpu_to_be32(cpu_addr);
>> descr->next = descr + 1;
>> descr->prev = descr - 1;
>> }
>> @@ -377,6 +379,7 @@ static int gelic_card_init_chain(struct gelic_card *card,
>> static int gelic_descr_prepare_rx(struct gelic_card *card,
>> struct gelic_descr *descr)
>> {
>> + dma_addr_t cpu_addr;
>> int offset;
>>
>> if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
>> @@ -398,11 +401,10 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>> if (offset)
>> skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
>> /* io-mmu-map the skb */
>> - descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
>> - descr->skb->data,
>> - gelic_rx_skb_size,
>> - DMA_FROM_DEVICE));
>> - if (!descr->buf_addr) {
>> + cpu_addr = dma_map_single(ctodev(card), descr->skb->data,
>> + gelic_rx_skb_size, DMA_FROM_DEVICE);
>> + descr->buf_addr = cpu_to_be32(cpu_addr);
>> + if (dma_mapping_error(ctodev(card), cpu_addr)) {
>> dev_kfree_skb_any(descr->skb);
>> descr->skb = NULL;
>> dev_info(ctodev(card),
>> @@ -782,7 +784,7 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
>>
>> buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
>>
>> - if (!buf) {
>> + if (dma_mapping_error(ctodev(card), buf)) {
>> dev_err(ctodev(card),
>> "dma map 2 failed (%p, %i). Dropping packet\n",
>> skb->data, skb->len);
>
> Looks good to me. The only tweak I would make is maybe using "dma_addr"
> , "phys_addr" or "bus_addr" instead of "cpu_addr", however that is more
> cosmetic than anything else.
Well, cpu_addr is used for cpu_to_be32(cpu_addr)...
> Reviewed-by: Alexander Duyck <alexanderduyck@fb•com>
Thanks for the review.
-Geoff
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-18 17:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-11 21:46 [PATCH net v8 0/2] net/ps3_gelic_net: DMA related fixes Geoff Levand
2023-03-11 21:46 ` [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length Geoff Levand
2023-03-11 22:37 ` Alexander H Duyck
2023-03-18 17:37 ` Geoff Levand
2023-03-11 21:46 ` [PATCH net v8 2/2] net/ps3_gelic_net: Use dma_mapping_error Geoff Levand
2023-03-11 22:41 ` Alexander H Duyck
2023-03-18 17:37 ` Geoff Levand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox