RE: [PATCH net-next] net: hns: Fix to conditionally convey RX checksum flag to stack

[Salil Mehta]

> -----Original Message-----
> From: David Miller [mailto:davem@xxxxxxxxxxxxx]
> Sent: Monday, November 28, 2016 5:13 PM
> To: Salil Mehta
> Cc: Zhuangyuzeng (Yisen); mehta.salil.lnk@xxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Linuxarm
> Subject: Re: [PATCH net-next] net: hns: Fix to conditionally convey RX
> checksum flag to stack
> 
> From: Salil Mehta <salil.mehta@xxxxxxxxxx>
> Date: Fri, 25 Nov 2016 13:32:40 +0000
> 
> > @@ -778,6 +778,35 @@ int hns_ae_get_regs_len(struct hnae_handle
> *handle)
> >  	return total_num;
> >  }
> >
> > +static bool hns_ae_is_l3l4_csum_err(struct hnae_handle *handle)
> > +{
> > +	struct hns_ppe_cb *ppe_cb = hns_get_ppe_cb(handle);
> > +	u32 regval;
> > +	bool retval = false;
> > +
> > +	/* read PPE_HIS_PRO_ERR register and check for the checksum
> errors */
> > +	regval = dsaf_read_dev(ppe_cb, PPE_HIS_PRO_ERR_REG);
> > +
> 
> I don't see how a single register can properly provide error status for
> a ring
> of pending received packets.
> 
> No matter how this register is implemented, it is either going to
> result in
> packets erroneously being marked as having errors, or error status
> being
> lost when multiple packets in a row have such errors.
> 
> For example, if you receive several packets in a row that have errors,
> you'll read this register for the first one.  If this read clears the
> error
> status, which I am guessing it does, then you won't see the error
> status
> for the next packet that had one of these errors as well.
Agreed David. I think I missed this part. This register is 
not well thought of and looks useless for checksum. Thanks
for identifying this!

> 
> If you don't have something which is provided on a per-packet basis
> then you can't determine the error properly.  Therefore you will just
> have to always ignore the checksum if there is any error indicated in
> the ring descriptor.
Yes, will float another patch ignoring the checksum.

Thanks
Salil