On 4/6/21 5:27 PM, Richard Cochran wrote:
On Tue, Apr 06, 2021 at 04:18:00PM -0700, Shannon Nelson wrote:
On 4/5/21 11:17 AM, Richard Cochran wrote:
On Mon, Apr 05, 2021 at 09:16:39AM -0700, Shannon Nelson wrote:
On 4/4/21 4:05 PM, Richard Cochran wrote:
This check is unneeded, because the ioctl layer never passes NULL here.
Yes, the ioctl layer never calls this with NULL, but we call it from within
the driver when we spin operations back up after a FW reset.
So why not avoid the special case and pass a proper request?
We do this because our firmware reset path is a special case that we have to
handle, and we do so by replaying the previous configuration request.
Passing the NULL request gives the code the ability to watch for this case
while keeping the special case handling simple: the code that drives the
replay logic doesn't need to know the hwstamp details, it just needs to
signal the replay and let the hwstamp code keep track of its own data and
request history.
I can update the comment to make that replay case more obvious.
No, please, I am asking you to provide a hwtstamp_config from your
driver. What is so hard about that?
What I think you are asking is that we not extend the current assumption
that *ifr will always be a useful pointer, a perfectly reasonable
request. Our ioctl() handler follows this as expected. It is our
internal usage that might look like an extension.
We'd like to keep the replay related code and data to a minimum, and
this current implementation is a simple way to do so, keeping the state
and config info within the ptp side.
I suppose one alternative is that we pull the copy_from_user() bits into
ionic_ioctl() and hand a hwtstamp_config struct pointer to
ionic_lif_hwstamp_set(). In our reset case we can either hand a NULL
pointer to ionic_lif_hwstamp_set() or add a reset parameter to the call
where ionic_lif_hwstamp_set() simply won't inspect the hwtstamp_config
pointer.
Another alternative would be to split the top layer of
ionic_lif_hwstamp_set() into two different functions, one called by the
ioctl handler with an *ifr, the other called by the fw reset logic
without an *ifr, that both call into the remaining with a
hwtstamp_config pointer. This seems somewhat similar to the ixgbe approach.
Either way, in the end it seems like extra code to get to the same result.
sln
