| Message ID | 1455026303-17014-1-git-send-email-grygorii.strashko@ti.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Grygorii Strashko > Sent: 09 February 2016 13:58 > From: Arnd Bergmann <arnd@arndb.de> > > The commit 899077791403 ("netcp: try to reduce type confusion in descriptors") > introduces a regression in Kernel 4.5-rc1 and it breaks > get/set_pad_info() functionality. > > The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to > store DMA/MEM buffer pointer and buffer size respectively. And in both > cases for Keystone 2 the pointer type size is 32 bit regardless of > LAPE enabled or not, because CONFIG_ARCH_DMA_ADDR_T_64BIT originally > is not expected to be defined. > > !LAPE LPAE > sizeof(void*) 32bit 32bit > sizeof(dma_addr_t) 32bit 32bit > sizeof(phys_addr_t) 32bit 64bit > > Unfortunately, above commit changed buffer's pointers save/restore > code (get/set_pad_info()) and added intermediate conversation to u64 > which works incorrectly on 32bit Keystone 2 and causes TI NETCP driver > crash in RX/TX path due to "Unable to handle kernel NULL pointer" > exception. This issue was reported and discussed in [1]. > > Hence, fix it by partially reverting above commit and restoring > get/set_pad_info() functionality as it was before. You should really get rid of most of the horrid pointer-integer casts. Code like: > void *buf_ptr; ... > + get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc); is just asking for trouble. You'd be better using assignments like: buf_ptr = (cast)get_pad_0(ndesc); buf_len = get_pad_1(ndesc); Then the values are passed (and cast) as numerics. In reality the 'pad' fields ought to be renamed - since they aren't pads. Perhaps they should be a union? David
>-----Original Message----- >From: David Laight [mailto:David.Laight@ACULAB.COM] >Sent: Tuesday, February 09, 2016 9:13 AM >To: Strashko, Grygorii; netdev@vger.kernel.org; David S . Miller; Arnd Bergmann >Cc: Cooper Jr., Franklin; Nori, Sekhar; linux-kernel@vger.kernel.org; Kwok, WingMan; >Karicheri, Muralidharan; N, Mugunthan V >Subject: RE: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality > >From: Grygorii Strashko >> Sent: 09 February 2016 13:58 >> From: Arnd Bergmann <arnd@arndb.de> >> >> The commit 899077791403 ("netcp: try to reduce type confusion in >> descriptors") introduces a regression in Kernel 4.5-rc1 and it breaks >> get/set_pad_info() functionality. >> >> The TI NETCP driver uses pad0 and pad1 fields of knav_dma_desc to >> store DMA/MEM buffer pointer and buffer size respectively. And in both >> cases for Keystone 2 the pointer type size is 32 bit regardless of >> LAPE enabled or not, because CONFIG_ARCH_DMA_ADDR_T_64BIT originally >> is not expected to be defined. >> >> !LAPE LPAE >> sizeof(void*) 32bit 32bit >> sizeof(dma_addr_t) 32bit 32bit >> sizeof(phys_addr_t) 32bit 64bit >> >> Unfortunately, above commit changed buffer's pointers save/restore >> code (get/set_pad_info()) and added intermediate conversation to u64 >> which works incorrectly on 32bit Keystone 2 and causes TI NETCP driver >> crash in RX/TX path due to "Unable to handle kernel NULL pointer" >> exception. This issue was reported and discussed in [1]. >> >> Hence, fix it by partially reverting above commit and restoring >> get/set_pad_info() functionality as it was before. > >You should really get rid of most of the horrid pointer-integer casts. >Code like: >> void *buf_ptr; >... >> + get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc); >is just asking for trouble. > >You'd be better using assignments like: > buf_ptr = (cast)get_pad_0(ndesc); > buf_len = get_pad_1(ndesc); >Then the values are passed (and cast) as numerics. > >In reality the 'pad' fields ought to be renamed - since they aren't pads. >Perhaps they should be a union? No. At the end of the descriptor, host software can add scratchpad which is not modified by the hardware, but is used by the driver. So please don't rename. Murali > > David
From: Karicheri, Muralidharan > Sent: 09 February 2016 16:10 ... > >In reality the 'pad' fields ought to be renamed - since they aren't pads. > >Perhaps they should be a union? > No. At the end of the descriptor, host software can add scratchpad which is > not modified by the hardware, but is used by the driver. So please don't > rename. So comment in the definition that the hardware doesn't modify them. The driver is defining these fields and they are definitely NOT padding. David
Murali Karicheri Linux Kernel, Software Development >-----Original Message----- >From: David Laight [mailto:David.Laight@ACULAB.COM] >Sent: Tuesday, February 09, 2016 11:10 AM >To: Karicheri, Muralidharan; Strashko, Grygorii; netdev@vger.kernel.org; David S . Miller; >Arnd Bergmann >Cc: Cooper Jr., Franklin; Nori, Sekhar; linux-kernel@vger.kernel.org; Kwok, WingMan; N, >Mugunthan V >Subject: RE: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality > >From: Karicheri, Muralidharan >> Sent: 09 February 2016 16:10 >... >> >In reality the 'pad' fields ought to be renamed - since they aren't pads. >> >Perhaps they should be a union? > >> No. At the end of the descriptor, host software can add scratchpad >> which is not modified by the hardware, but is used by the driver. So >> please don't rename. > >So comment in the definition that the hardware doesn't modify them. >The driver is defining these fields and they are definitely NOT padding. It is scratch pad, not padding. Looks like pad is a confusing name. Can be renamed to sw_data to be in sync with spec below. The hardware spec from http://www.ti.com/lit/ug/sprugr9h/sprugr9h.pdf The other SW data portion of the descriptor exists after all of the defined words and is reserved for use by the host software to store completely private data. This region is not used in any way by the DMA or queue manager modules in a Multicore Navigator system and these modules will not modify any bytes within this region. Murali > > David
From: Karicheri, Muralidharan > Sent: 09 February 2016 16:19 > >... > >> >In reality the 'pad' fields ought to be renamed - since they aren't pads. > >> >Perhaps they should be a union? > > > >> No. At the end of the descriptor, host software can add scratchpad > >> which is not modified by the hardware, but is used by the driver. So > >> please don't rename. > > > >So comment in the definition that the hardware doesn't modify them. > >The driver is defining these fields and they are definitely NOT padding. > > > It is scratch pad, not padding. Looks like pad is a confusing name. Can be > renamed to sw_data to be in sync with spec below. > > The hardware spec from > http://www.ti.com/lit/ug/sprugr9h/sprugr9h.pdf > > The other SW data portion of the descriptor exists after all of the defined > words and is reserved for use by the host software to store completely > private data. This region is not used in any way by the DMA or queue manager > modules in a Multicore Navigator system and these modules will not modify > any bytes within this region. Right, so comment that the hardware doesn't look at the fields. But name/type the structure fields to indicate what they contain. Maybe sw_buf_len etc - but I suspect there are much more meaningful names. David
>-----Original Message----- >From: David Laight [mailto:David.Laight@ACULAB.COM] >Sent: Tuesday, February 09, 2016 11:38 AM >To: Karicheri, Muralidharan; Strashko, Grygorii; netdev@vger.kernel.org; David S . Miller; >Arnd Bergmann >Cc: Cooper Jr., Franklin; Nori, Sekhar; linux-kernel@vger.kernel.org; Kwok, WingMan; N, >Mugunthan V >Subject: RE: [PATCH] net: ti: netcp: restore get/set_pad_info() functionality > >From: Karicheri, Muralidharan >> Sent: 09 February 2016 16:19 >> >... >> >> >In reality the 'pad' fields ought to be renamed - since they aren't pads. >> >> >Perhaps they should be a union? >> > >> >> No. At the end of the descriptor, host software can add scratchpad >> >> which is not modified by the hardware, but is used by the driver. >> >> So please don't rename. >> > >> >So comment in the definition that the hardware doesn't modify them. >> >The driver is defining these fields and they are definitely NOT padding. >> >> >> It is scratch pad, not padding. Looks like pad is a confusing name. >> Can be renamed to sw_data to be in sync with spec below. >> >> The hardware spec from >> http://www.ti.com/lit/ug/sprugr9h/sprugr9h.pdf >> >> The other SW data portion of the descriptor exists after all of the >> defined words and is reserved for use by the host software to store >> completely private data. This region is not used in any way by the DMA >> or queue manager modules in a Multicore Navigator system and these >> modules will not modify any bytes within this region. > >Right, so comment that the hardware doesn't look at the fields. >But name/type the structure fields to indicate what they contain. >Maybe sw_buf_len etc - but I suspect there are much more meaningful names. The descriptors are usable by different drivers, one driver may use it as buf ptr/ len, other for something else. So they should remain as generic and it is up to individual drivers to use it in whatever way it requires. My suggestion is to rename pad field in struct knav_dma_desc to sw_data to avoid confusion. i.e + __le32 pad[4]; to + __le32 sw_data[4]; Murali > > David
On Tuesday 09 February 2016 16:55:42 Karicheri, Muralidharan wrote: > > The descriptors are usable by different drivers, one driver may use it as > buf ptr/ len, other for something else. So they should remain as generic > and it is up to individual drivers to use it in whatever way it requires. > My suggestion is to rename pad field in struct knav_dma_desc to sw_data > to avoid confusion. i.e > > + __le32 pad[4]; > > to > > + __le32 sw_data[4]; > If the hardware doesn't access them, they can probably just be u32 and not do any byte swapping. Arnd
Hi All, On 02/09/2016 09:38 PM, Arnd Bergmann wrote: > On Tuesday 09 February 2016 16:55:42 Karicheri, Muralidharan wrote: >> >> The descriptors are usable by different drivers, one driver may use it as >> buf ptr/ len, other for something else. So they should remain as generic >> and it is up to individual drivers to use it in whatever way it requires. >> My suggestion is to rename pad field in struct knav_dma_desc to sw_data >> to avoid confusion. i.e >> >> + __le32 pad[4]; >> >> to >> >> + __le32 sw_data[4]; >> > > If the hardware doesn't access them, they can probably just be u32 > and not do any byte swapping. > Most of propositions, mentioned in this thread (and not only), sounds reasonable, and there are definitely a lot of optimization/cleanups and refactorings which could be done for this driver (of course it's up to NETCP maintainers to decide). But this particular patch is intended to fix regression and restore Keystone 2 networking functionality, *including NFS boot*, when vanilla's config files are used (keystone_defconfig, multi_v7_defconfig). And this is "not" an optimization or cleanup - this is partial revert of buggy commit. So, could we fix regression first, pleeeaaase? Then other things can be done if someone have time and would like to take the initiative (and would be ready to get at minimum "Tested-by" tag from netcp maintainers :)).
I would like some of the feedback to be taken into consideration and integrated into this patch. Part of the reason this regression was introduced was probably because the purpose of some fields or descriptor semantics was not defined properly. Therefore it is absolutely appropriate to properly name and document these fields as part of the bug fix. Thank you.
On 02/09/2016 02:38 PM, Arnd Bergmann wrote: > On Tuesday 09 February 2016 16:55:42 Karicheri, Muralidharan wrote: >> The descriptors are usable by different drivers, one driver may use it as >> buf ptr/ len, other for something else. So they should remain as generic >> and it is up to individual drivers to use it in whatever way it requires. >> My suggestion is to rename pad field in struct knav_dma_desc to sw_data >> to avoid confusion. i.e >> >> + __le32 pad[4]; >> >> to >> >> + __le32 sw_data[4]; >> > If the hardware doesn't access them, they can probably just be u32 > and not do any byte swapping. Agree. > > Arnd >
On 02/09/2016 02:38 PM, Arnd Bergmann wrote: > On Tuesday 09 February 2016 16:55:42 Karicheri, Muralidharan wrote: >> >> The descriptors are usable by different drivers, one driver may use it as >> buf ptr/ len, other for something else. So they should remain as generic >> and it is up to individual drivers to use it in whatever way it requires. >> My suggestion is to rename pad field in struct knav_dma_desc to sw_data >> to avoid confusion. i.e >> >> + __le32 pad[4]; >> >> to >> >> + __le32 sw_data[4]; >> > > If the hardware doesn't access them, they can probably just be u32 > and not do any byte swapping. > Ok. Will do in v1 > Arnd >
On 02/16/2016 03:24 PM, David Miller wrote: > > I would like some of the feedback to be taken into consideration and > integrated into this patch. > > Part of the reason this regression was introduced was probably because > the purpose of some fields or descriptor semantics was not defined > properly. > > Therefore it is absolutely appropriate to properly name and document > these fields as part of the bug fix. > > Thank you. > David, I will take over this from Grygorii as he is out of office. I propose to keep this patch as is and add additional patch to address the feedback in the same series (v1). Is that fine with you? There mainly two feedbacks. 1. Rename the pad to something meaningful. I had suggested to use the word sw_data to match with what is in the hw spec. Also since this field is not touched by hardware, I will change the type to u32 as suggested by Arnd. 2. Comment about type cast. I will investigate and make update or discuss this further
From: Murali Karicheri <m-karicheri2@ti.com> Date: Thu, 18 Feb 2016 12:13:10 -0500 > On 02/16/2016 03:24 PM, David Miller wrote: >> >> I would like some of the feedback to be taken into consideration and >> integrated into this patch. >> >> Part of the reason this regression was introduced was probably because >> the purpose of some fields or descriptor semantics was not defined >> properly. >> >> Therefore it is absolutely appropriate to properly name and document >> these fields as part of the bug fix. >> >> Thank you. >> > David, > > I will take over this from Grygorii as he is out of office. > > I propose to keep this patch as is and add additional patch to address > the feedback in the same series (v1). Is that fine with you? No, I want the code to be clarified along with the bug fix so that this bug is unlikely to resurface.
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c index c61d66d..0b26e52 100644 --- a/drivers/net/ethernet/ti/netcp_core.c +++ b/drivers/net/ethernet/ti/netcp_core.c @@ -117,20 +117,10 @@ static void get_pkt_info(dma_addr_t *buff, u32 *buff_len, dma_addr_t *ndesc, *ndesc = le32_to_cpu(desc->next_desc); } -static void get_pad_info(u32 *pad0, u32 *pad1, u32 *pad2, struct knav_dma_desc *desc) +static void get_pad_info(u32 *pad0, u32 *pad1, struct knav_dma_desc *desc) { *pad0 = le32_to_cpu(desc->pad[0]); *pad1 = le32_to_cpu(desc->pad[1]); - *pad2 = le32_to_cpu(desc->pad[2]); -} - -static void get_pad_ptr(void **padptr, struct knav_dma_desc *desc) -{ - u64 pad64; - - pad64 = le32_to_cpu(desc->pad[0]) + - ((u64)le32_to_cpu(desc->pad[1]) << 32); - *padptr = (void *)(uintptr_t)pad64; } static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len, @@ -163,11 +153,10 @@ static void set_desc_info(u32 desc_info, u32 pkt_info, desc->packet_info = cpu_to_le32(pkt_info); } -static void set_pad_info(u32 pad0, u32 pad1, u32 pad2, struct knav_dma_desc *desc) +static void set_pad_info(u32 pad0, u32 pad1, struct knav_dma_desc *desc) { desc->pad[0] = cpu_to_le32(pad0); desc->pad[1] = cpu_to_le32(pad1); - desc->pad[2] = cpu_to_le32(pad1); } static void set_org_pkt_info(dma_addr_t buff, u32 buff_len, @@ -581,7 +570,6 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp, dma_addr_t dma_desc, dma_buf; unsigned int buf_len, dma_sz = sizeof(*ndesc); void *buf_ptr; - u32 pad[2]; u32 tmp; get_words(&dma_desc, 1, &desc->next_desc); @@ -593,14 +581,12 @@ static void netcp_free_rx_desc_chain(struct netcp_intf *netcp, break; } get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc); - get_pad_ptr(&buf_ptr, ndesc); + get_pad_info((u32 *)&buf_ptr, &buf_len, ndesc); dma_unmap_page(netcp->dev, dma_buf, PAGE_SIZE, DMA_FROM_DEVICE); __free_page(buf_ptr); knav_pool_desc_put(netcp->rx_pool, desc); } - - get_pad_info(&pad[0], &pad[1], &buf_len, desc); - buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32)); + get_pad_info((u32 *)&buf_ptr, &buf_len, desc); if (buf_ptr) netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr); @@ -639,8 +625,8 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp) dma_addr_t dma_desc, dma_buff; struct netcp_packet p_info; struct sk_buff *skb; - u32 pad[2]; void *org_buf_ptr; + u32 tmp; dma_desc = knav_queue_pop(netcp->rx_queue, &dma_sz); if (!dma_desc) @@ -653,8 +639,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp) } get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc); - get_pad_info(&pad[0], &pad[1], &org_buf_len, desc); - org_buf_ptr = (void *)(uintptr_t)(pad[0] + ((u64)pad[1] << 32)); + get_pad_info((u32 *)&org_buf_ptr, &org_buf_len, desc); if (unlikely(!org_buf_ptr)) { dev_err(netcp->ndev_dev, "NULL bufptr in desc\n"); @@ -679,7 +664,6 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp) /* Fill in the page fragment list */ while (dma_desc) { struct page *page; - void *ptr; ndesc = knav_pool_desc_unmap(netcp->rx_pool, dma_desc, dma_sz); if (unlikely(!ndesc)) { @@ -688,8 +672,7 @@ static int netcp_process_one_rx_packet(struct netcp_intf *netcp) } get_pkt_info(&dma_buff, &buf_len, &dma_desc, ndesc); - get_pad_ptr(&ptr, ndesc); - page = ptr; + get_pad_info((u32 *)&page, &tmp, ndesc); if (likely(dma_buff && buf_len && page)) { dma_unmap_page(netcp->dev, dma_buff, PAGE_SIZE, @@ -767,6 +750,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq) unsigned int buf_len, dma_sz; dma_addr_t dma; void *buf_ptr; + u32 tmp; /* Allocate descriptor */ while ((dma = knav_queue_pop(netcp->rx_fdq[fdq], &dma_sz))) { @@ -777,7 +761,7 @@ static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq) } get_org_pkt_info(&dma, &buf_len, desc); - get_pad_ptr(&buf_ptr, desc); + get_pad_info((u32 *)&buf_ptr, &tmp, desc); if (unlikely(!dma)) { dev_err(netcp->ndev_dev, "NULL orig_buff in desc\n"); @@ -829,7 +813,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq) struct page *page; dma_addr_t dma; void *bufptr; - u32 pad[3]; + u32 pad[2]; /* Allocate descriptor */ hwdesc = knav_pool_desc_get(netcp->rx_pool); @@ -846,7 +830,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq) SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); bufptr = netdev_alloc_frag(primary_buf_len); - pad[2] = primary_buf_len; + pad[1] = primary_buf_len; if (unlikely(!bufptr)) { dev_warn_ratelimited(netcp->ndev_dev, @@ -858,9 +842,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq) if (unlikely(dma_mapping_error(netcp->dev, dma))) goto fail; - pad[0] = lower_32_bits((uintptr_t)bufptr); - pad[1] = upper_32_bits((uintptr_t)bufptr); - + pad[0] = (u32)bufptr; } else { /* Allocate a secondary receive queue entry */ page = alloc_page(GFP_ATOMIC | GFP_DMA | __GFP_COLD); @@ -870,9 +852,8 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq) } buf_len = PAGE_SIZE; dma = dma_map_page(netcp->dev, page, 0, buf_len, DMA_TO_DEVICE); - pad[0] = lower_32_bits(dma); - pad[1] = upper_32_bits(dma); - pad[2] = 0; + pad[0] = (u32)page; + pad[1] = 0; } desc_info = KNAV_DMA_DESC_PS_INFO_IN_DESC; @@ -882,7 +863,7 @@ static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq) pkt_info |= (netcp->rx_queue_id & KNAV_DMA_DESC_RETQ_MASK) << KNAV_DMA_DESC_RETQ_SHIFT; set_org_pkt_info(dma, buf_len, hwdesc); - set_pad_info(pad[0], pad[1], pad[2], hwdesc); + set_pad_info(pad[0], pad[1], hwdesc); set_desc_info(desc_info, pkt_info, hwdesc); /* Push to FDQs */ @@ -971,11 +952,11 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp, unsigned int budget) { struct knav_dma_desc *desc; - void *ptr; struct sk_buff *skb; unsigned int dma_sz; dma_addr_t dma; int pkts = 0; + u32 tmp; while (budget--) { dma = knav_queue_pop(netcp->tx_compl_q, &dma_sz); @@ -988,8 +969,7 @@ static int netcp_process_tx_compl_packets(struct netcp_intf *netcp, continue; } - get_pad_ptr(&ptr, desc); - skb = ptr; + get_pad_info((u32 *)&skb, &tmp, desc); netcp_free_tx_desc_chain(netcp, desc, dma_sz); if (!skb) { dev_err(netcp->ndev_dev, "No skb in Tx desc\n"); @@ -1194,10 +1174,7 @@ static int netcp_tx_submit_skb(struct netcp_intf *netcp, } set_words(&tmp, 1, &desc->packet_info); - tmp = lower_32_bits((uintptr_t)&skb); - set_words(&tmp, 1, &desc->pad[0]); - tmp = upper_32_bits((uintptr_t)&skb); - set_words(&tmp, 1, &desc->pad[1]); + set_words((u32 *)&skb, 1, &desc->pad[0]); if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) { tmp = tx_pipe->switch_to_port;