diff mbox

[v2,3/3] net: netcp: rework the code for get/set sw_data in dma desc

Message ID 1455904724-17952-4-git-send-email-m-karicheri2@ti.com
State New
Headers show

Commit Message

Murali Karicheri Feb. 19, 2016, 5:58 p.m. UTC 
SW data field in descriptor can be used by software to hold private
data for the driver. As there are 4 words available for this purpose,
use separate macros to place it or retrieve the same to/from
descriptors. Also do type cast of data types accordingly.

Cc: Wingman Kwok <w-kwok2@ti.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Grygorii Strashko <grygorii.strashko@ti.com>
CC: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

---
 drivers/net/ethernet/ti/netcp_core.c | 72 +++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 17 deletions(-)

-- 
1.9.1

Comments

Arnd Bergmann Feb. 19, 2016, 8:55 p.m. UTC | #1 
On Friday 19 February 2016 12:58:44 Murali Karicheri wrote:
> SW data field in descriptor can be used by software to hold private

> data for the driver. As there are 4 words available for this purpose,

> use separate macros to place it or retrieve the same to/from

> descriptors. Also do type cast of data types accordingly.

> 

> Cc: Wingman Kwok <w-kwok2@ti.com>

> Cc: Mugunthan V N <mugunthanvnm@ti.com>

> CC: Arnd Bergmann <arnd@arndb.de>

> CC: Grygorii Strashko <grygorii.strashko@ti.com>

> CC: David Laight <David.Laight@ACULAB.COM>

> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>


Looks ok in principle.

Acked-by: Arnd Bergmann <arnd@arndb.de>


>  		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);

> -		get_sw_data((u32 *)&buf_ptr, &buf_len, ndesc);

> +		/* warning!!!! We are retrieving the virtual ptr in the sw_data

> +		 * field as a 32bit value. Will not work on 64bit machines

> +		 */

> +		buf_ptr = (void *)GET_SW_DATA0(ndesc);

> +		buf_len = (int)GET_SW_DATA1(desc);


I would have abstracted the retrieval of a pointer again,
and added the comment in the helper function once, it doesn't
really need to be duplicated everywhere.

	Arnd
Murali Karicheri Feb. 19, 2016, 10:21 p.m. UTC | #2 
On 02/19/2016 03:55 PM, Arnd Bergmann wrote:
> On Friday 19 February 2016 12:58:44 Murali Karicheri wrote:

>> SW data field in descriptor can be used by software to hold private

>> data for the driver. As there are 4 words available for this purpose,

>> use separate macros to place it or retrieve the same to/from

>> descriptors. Also do type cast of data types accordingly.

>>

>> Cc: Wingman Kwok <w-kwok2@ti.com>

>> Cc: Mugunthan V N <mugunthanvnm@ti.com>

>> CC: Arnd Bergmann <arnd@arndb.de>

>> CC: Grygorii Strashko <grygorii.strashko@ti.com>

>> CC: David Laight <David.Laight@ACULAB.COM>

>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

> 

> Looks ok in principle.

> 

> Acked-by: Arnd Bergmann <arnd@arndb.de>

> 

>>  		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);

>> -		get_sw_data((u32 *)&buf_ptr, &buf_len, ndesc);

>> +		/* warning!!!! We are retrieving the virtual ptr in the sw_data

>> +		 * field as a 32bit value. Will not work on 64bit machines

>> +		 */

>> +		buf_ptr = (void *)GET_SW_DATA0(ndesc);

>> +		buf_len = (int)GET_SW_DATA1(desc);

> 

> I would have abstracted the retrieval of a pointer again,

> and added the comment in the helper function once, it doesn't

> really need to be duplicated everywhere.

> 

Arnd,

I thought about it to add it to the API. API currently set buffer
and ptr. It would be an issue only if store/retrieve ptr in/from the sw_data.
So for the comment to be really useful to someone who is changing the code,
doesn't it make sense to add it at the point of invocation as done in this
patch? No?

Murali

> 	Arnd

> 



-- 
Murali Karicheri
Linux Kernel, Keystone
Arnd Bergmann Feb. 19, 2016, 10:25 p.m. UTC | #3 
On Friday 19 February 2016 17:21:57 Murali Karicheri wrote:
> >>              get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);

> >> -            get_sw_data((u32 *)&buf_ptr, &buf_len, ndesc);

> >> +            /* warning!!!! We are retrieving the virtual ptr in the sw_data

> >> +             * field as a 32bit value. Will not work on 64bit machines

> >> +             */

> >> +            buf_ptr = (void *)GET_SW_DATA0(ndesc);

> >> +            buf_len = (int)GET_SW_DATA1(desc);

> > 

> > I would have abstracted the retrieval of a pointer again,

> > and added the comment in the helper function once, it doesn't

> > really need to be duplicated everywhere.

> > 

> Arnd,

> 

> I thought about it to add it to the API. API currently set buffer

> and ptr. It would be an issue only if store/retrieve ptr in/from the sw_data.

> So for the comment to be really useful to someone who is changing the code,

> doesn't it make sense to add it at the point of invocation as done in this

> patch? No?

> 


Up to you, it was just an idea and you have my Ack either way.

	Arnd
Murali Karicheri Feb. 22, 2016, 5:04 p.m. UTC | #4 
On 02/19/2016 05:25 PM, Arnd Bergmann wrote:
> On Friday 19 February 2016 17:21:57 Murali Karicheri wrote:

>>>>              get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);

>>>> -            get_sw_data((u32 *)&buf_ptr, &buf_len, ndesc);

>>>> +            /* warning!!!! We are retrieving the virtual ptr in the sw_data

>>>> +             * field as a 32bit value. Will not work on 64bit machines

>>>> +             */

>>>> +            buf_ptr = (void *)GET_SW_DATA0(ndesc);

>>>> +            buf_len = (int)GET_SW_DATA1(desc);

>>>

>>> I would have abstracted the retrieval of a pointer again,

>>> and added the comment in the helper function once, it doesn't

>>> really need to be duplicated everywhere.

>>>

>> Arnd,

>>

>> I thought about it to add it to the API. API currently set buffer

>> and ptr. It would be an issue only if store/retrieve ptr in/from the sw_data.

>> So for the comment to be really useful to someone who is changing the code,

>> doesn't it make sense to add it at the point of invocation as done in this

>> patch? No?

>>

> 

> Up to you, it was just an idea and you have my Ack either way.

> 

> 	Arnd

> 

Ok. Thanks

-- 
Murali Karicheri
Linux Kernel, Keystone
diff mbox

Patch

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 84bab29..029841f 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -117,13 +117,18 @@  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_sw_data(u32 *data0, u32 *data1, struct knav_dma_desc *desc)
+static u32 get_sw_data(int index, struct knav_dma_desc *desc)
 {
 	/* No Endian conversion needed as this data is untouched by hw */
-	*data0 = desc->sw_data[0];
-	*data1 = desc->sw_data[1];
+	return desc->sw_data[index];
 }
 
+/* use these macros to get sw data */
+#define GET_SW_DATA0(desc) get_sw_data(0, desc)
+#define GET_SW_DATA1(desc) get_sw_data(1, desc)
+#define GET_SW_DATA2(desc) get_sw_data(2, desc)
+#define GET_SW_DATA3(desc) get_sw_data(3, desc)
+
 static void get_org_pkt_info(dma_addr_t *buff, u32 *buff_len,
 			     struct knav_dma_desc *desc)
 {
@@ -154,13 +159,18 @@  static void set_desc_info(u32 desc_info, u32 pkt_info,
 	desc->packet_info = cpu_to_le32(pkt_info);
 }
 
-static void set_sw_data(u32 data0, u32 data1, struct knav_dma_desc *desc)
+static void set_sw_data(int index, u32 data, struct knav_dma_desc *desc)
 {
 	/* No Endian conversion needed as this data is untouched by hw */
-	desc->sw_data[0] = data0;
-	desc->sw_data[1] = data1;
+	desc->sw_data[index] = data;
 }
 
+/* use these macros to set sw data */
+#define SET_SW_DATA0(data, desc) set_sw_data(0, data, desc)
+#define SET_SW_DATA1(data, desc) set_sw_data(1, data, desc)
+#define SET_SW_DATA2(data, desc) set_sw_data(2, data, desc)
+#define SET_SW_DATA3(data, desc) set_sw_data(3, data, desc)
+
 static void set_org_pkt_info(dma_addr_t buff, u32 buff_len,
 			     struct knav_dma_desc *desc)
 {
@@ -583,12 +593,20 @@  static void netcp_free_rx_desc_chain(struct netcp_intf *netcp,
 			break;
 		}
 		get_pkt_info(&dma_buf, &tmp, &dma_desc, ndesc);
-		get_sw_data((u32 *)&buf_ptr, &buf_len, ndesc);
+		/* warning!!!! We are retrieving the virtual ptr in the sw_data
+		 * field as a 32bit value. Will not work on 64bit machines
+		 */
+		buf_ptr = (void *)GET_SW_DATA0(ndesc);
+		buf_len = (int)GET_SW_DATA1(desc);
 		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_sw_data((u32 *)&buf_ptr, &buf_len, desc);
+	/* warning!!!! We are retrieving the virtual ptr in the sw_data
+	 * field as a 32bit value. Will not work on 64bit machines
+	 */
+	buf_ptr = (void *)GET_SW_DATA0(desc);
+	buf_len = (int)GET_SW_DATA1(desc);
 
 	if (buf_ptr)
 		netcp_frag_free(buf_len <= PAGE_SIZE, buf_ptr);
@@ -628,7 +646,6 @@  static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	struct netcp_packet p_info;
 	struct sk_buff *skb;
 	void *org_buf_ptr;
-	u32 tmp;
 
 	dma_desc = knav_queue_pop(netcp->rx_queue, &dma_sz);
 	if (!dma_desc)
@@ -641,7 +658,11 @@  static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 	}
 
 	get_pkt_info(&dma_buff, &buf_len, &dma_desc, desc);
-	get_sw_data((u32 *)&org_buf_ptr, &org_buf_len, desc);
+	/* warning!!!! We are retrieving the virtual ptr in the sw_data
+	 * field as a 32bit value. Will not work on 64bit machines
+	 */
+	org_buf_ptr = (void *)GET_SW_DATA0(desc);
+	org_buf_len = (int)GET_SW_DATA1(desc);
 
 	if (unlikely(!org_buf_ptr)) {
 		dev_err(netcp->ndev_dev, "NULL bufptr in desc\n");
@@ -674,7 +695,10 @@  static int netcp_process_one_rx_packet(struct netcp_intf *netcp)
 		}
 
 		get_pkt_info(&dma_buff, &buf_len, &dma_desc, ndesc);
-		get_sw_data((u32 *)&page, &tmp, ndesc);
+		/* warning!!!! We are retrieving the virtual ptr in the sw_data
+		 * field as a 32bit value. Will not work on 64bit machines
+		 */
+		page = (struct page *)GET_SW_DATA0(desc);
 
 		if (likely(dma_buff && buf_len && page)) {
 			dma_unmap_page(netcp->dev, dma_buff, PAGE_SIZE,
@@ -752,7 +776,6 @@  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))) {
@@ -763,7 +786,10 @@  static void netcp_free_rx_buf(struct netcp_intf *netcp, int fdq)
 		}
 
 		get_org_pkt_info(&dma, &buf_len, desc);
-		get_sw_data((u32 *)&buf_ptr, &tmp, desc);
+		/* warning!!!! We are retrieving the virtual ptr in the sw_data
+		 * field as a 32bit value. Will not work on 64bit machines
+		 */
+		buf_ptr = (void *)GET_SW_DATA0(desc);
 
 		if (unlikely(!dma)) {
 			dev_err(netcp->ndev_dev, "NULL orig_buff in desc\n");
@@ -844,6 +870,9 @@  static int netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		if (unlikely(dma_mapping_error(netcp->dev, dma)))
 			goto fail;
 
+		/* warning!!!! We are saving the virtual ptr in the sw_data
+		 * field as a 32bit value. Will not work on 64bit machines
+		 */
 		sw_data[0] = (u32)bufptr;
 	} else {
 		/* Allocate a secondary receive queue entry */
@@ -854,6 +883,9 @@  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);
+		/* warning!!!! We are saving the virtual ptr in the sw_data
+		 * field as a 32bit value. Will not work on 64bit machines
+		 */
 		sw_data[0] = (u32)page;
 		sw_data[1] = 0;
 	}
@@ -865,7 +897,8 @@  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_sw_data(sw_data[0], sw_data[1], hwdesc);
+	SET_SW_DATA0(sw_data[0], hwdesc);
+	SET_SW_DATA1(sw_data[1], hwdesc);
 	set_desc_info(desc_info, pkt_info, hwdesc);
 
 	/* Push to FDQs */
@@ -958,7 +991,6 @@  static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 	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);
@@ -971,7 +1003,10 @@  static int netcp_process_tx_compl_packets(struct netcp_intf *netcp,
 			continue;
 		}
 
-		get_sw_data((u32 *)&skb, &tmp, desc);
+		/* warning!!!! We are retrieving the virtual ptr in the sw_data
+		 * field as a 32bit value. Will not work on 64bit machines
+		 */
+		skb = (struct sk_buff *)GET_SW_DATA0(desc);
 		netcp_free_tx_desc_chain(netcp, desc, dma_sz);
 		if (!skb) {
 			dev_err(netcp->ndev_dev, "No skb in Tx desc\n");
@@ -1176,7 +1211,10 @@  static int netcp_tx_submit_skb(struct netcp_intf *netcp,
 	}
 
 	set_words(&tmp, 1, &desc->packet_info);
-	set_sw_data((u32)skb, 0, desc);
+	/* warning!!!! We are saving the virtual ptr in the sw_data
+	 * field as a 32bit value. Will not work on 64bit machines
+	 */
+	SET_SW_DATA0((u32)skb, desc);
 
 	if (tx_pipe->flags & SWITCH_TO_PORT_IN_TAGINFO) {
 		tmp = tx_pipe->switch_to_port;