Re: [PATCH linux dev-4.13 2/4] fsi/occ: Add Retries on checksum errors

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2018-05-21 at 13:58 -0500, Eddie James wrote:
> 
> The checksum is part of the OCC response, so it's not a transport thing. 
> If we've gotten to checking the checksum then we've got a full response 
> that looks valid so far (reasonable length, etc).

I have had cases of transport errors causing a bad checksum, CRC4 is a
bit weak at the FSI transport level.

> If we're trying to adhere to the OCC spec, then I'm of the opinion that 
> we shouldn't do any retries except for those handled in the occ-hwmon 
> driver.
> 
> > > 
> > > Ben: Did you actually hit cases where this path was triggered? There
> > > was the corruption issue with simultaneous LPC cycles that turned out
> > > to be issues around level-shifters and synchronisers, was that it?
> > 
> > Yes, and I had cases where the CRC4 didn't "catch" the errors. The
> > retry fixed it. Now with the FSI layer being much more reliable, it
> > might be that all that retry stuff I added is no longer necessary, so I
> > won't be fighting for it, though I did find the upper layer error
> > handling to be somewhat lacking in efficacy...
> > 
> > I plan to do a deep dive on the rest of the OCC driver this week
> > regardless. I don't like a few things about it, such as the 2 layers
> > between fsi-occ and sbe_p9, that should be just one (sadly this change
> > will break the userspace binding code...).
> 
> What do you mean two layers? fsi-occ and occ-hwmon? I fear that the 
> hwmon maintainer won't like having so much transport stuff (and a 
> chardev) in the hwmon driver.

The transport stuff in fsi-occ can be brought down to almost nothing,
most of the code in there is ... not particularily useful.

As for the chardev, other drivers in hwmon do that too, but I suppose I
can handle the discussion with Guenter.

Cheers,
Ben.

> > 
> > I'll see if I can figure out how that error hanlding works.
> > 
> > Cheers,
> > Ben.
> > 
> > > >   done:
> > > >   	mutex_unlock(&occ->occ_lock);
> > > >   
> > > > @@ -732,6 +735,7 @@ static void occ_worker(struct work_struct *work)
> > > >   	clear_bit(XFR_IN_PROGRESS, &xfr->flags);
> > > >   	list_del(&xfr->link);
> > > >   	empty = list_empty(&occ->xfrs);
> > > > +	retries = 0;
> > > >   
> > > >   	spin_unlock_irqrestore(&occ->list_lock, flags);
> > > >   
> > > > -- 
> > > > 2.17.0
> > > >