Hi benh,<br><br> Please find my comments inline.<br><br>Thanks and regards,<br>SathyaNarayanan<br><br><div class="gmail_quote">On Tue, Jun 24, 2008 at 4:50 AM, Benjamin Herrenschmidt <<a href="mailto:benh@kernel.crashing.org" target="_blank">benh@kernel.crashing.org</a>> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div>On Mon, 2008-06-23 at 14:54 +0200, Stefan Roese wrote:<br>
> From: Sathya Narayanan <<a href="mailto:sathyan@teamf1.com" target="_blank">sathyan@teamf1.com</a>><br>
><br>
> The descriptor pointers were not initialized to NIL values, so it was<br>
> poiniting to some random addresses which was completely invalid. This<br>
> fix takes care of initializing the descriptor to NIL values and clearing<br>
> the valid descriptors on clean ring operation.<br>
><br>
> Signed-off-by: Sathya Narayanan <<a href="mailto:sathyan@teamf1.com" target="_blank">sathyan@teamf1.com</a>><br>
> Signed-off-by: Stefan Roese <<a href="mailto:sr@denx.de" target="_blank">sr@denx.de</a>><br>
> ---<br>
> drivers/net/ibm_newemac/core.c | 6 +++++-<br>
> 1 files changed, 5 insertions(+), 1 deletions(-)<br>
><br>
> diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c<br>
> index 5d2108c..6dfc2c9 100644<br>
> --- a/drivers/net/ibm_newemac/core.c<br>
> +++ b/drivers/net/ibm_newemac/core.c<br>
> @@ -1025,7 +1025,7 @@ static void emac_clean_tx_ring(struct emac_instance *dev)<br>
> int i;<br>
><br>
> for (i = 0; i < NUM_TX_BUFF; ++i) {<br>
> - if (dev->tx_skb[i]) {<br>
> + if (dev->tx_skb[i] && dev->tx_desc[i].data_ptr) {<br>
<br>
</div>Why changing the test above ?</blockquote><div><br> The reason for changing this condition is , In any of the case if the dev->tx_skb is not containing valid address, Then while clearing it you may be resulted in "address voilations". This additional condition ensures that we are clearing the valid skbs.<br>
Further this condition is not in general data flow, So this additional condition should not have any impact on performance.<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<div><br>
> dev_kfree_skb(dev->tx_skb[i]);<br>
> dev->tx_skb[i] = NULL;<br>
> if (dev->tx_desc[i].ctrl & MAL_TX_CTRL_READY)<br>
> @@ -2719,6 +2719,10 @@ static int __devinit emac_probe(struct of_device *ofdev,<br>
> /* Clean rings */<br>
> memset(dev->tx_desc, 0, NUM_TX_BUFF * sizeof(struct mal_descriptor));<br>
> memset(dev->rx_desc, 0, NUM_RX_BUFF * sizeof(struct mal_descriptor));<br>
> + for (i = 0; i <= NUM_TX_BUFF; i++)<br>
> + dev->tx_skb[i] = NULL;<br>
> + for (i = 0; i <= NUM_RX_BUFF; i++)<br>
> + dev->rx_skb[i] = NULL;<br>
<br>
</div>Why not use memset here too ?</blockquote><div> Yes, It was valid to use memset here. I can send the modified patch for it. <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<div><div></div><div><br>
> /* Attach to ZMII, if needed */<br>
> if (emac_has_feature(dev, EMAC_FTR_HAS_ZMII) &&<br>
<br>
</div></div></blockquote></div><br>