On Mon, Jan 08, 2024 at 01:47:12PM +0100, Karol Kolacinski wrote: Should there be a "From: Jacob" line here to match the Signed-off-by below?
> Add PTP state machine so that the driver can correctly identify PTP > state around resets. > When the driver got information about ungraceful reset, PTP was not > prepared for reset and it returned error. When this situation occurs, > prepare PTP before rebuilding its structures. > > Signed-off-by: Jacob Keller <[email protected]> > Signed-off-by: Karol Kolacinski <[email protected]> > Reviewed-by: Jacob Keller <[email protected]> Hi Karol and Jacob, FWIIW, The combination of both a Signed-off-by and Reviewed-by tag from Jacob seems a little odd to me. If he authored the patch then I would have gone with the following (along with the From line mentioned above): Signed-off-by: Jacob Keller <[email protected]> Signed-off-by: Karol Kolacinski <[email protected]> Otherwise, if he reviewed the patch I would have gone with: Reviewed-by: Jacob Keller <[email protected]> Signed-off-by: Karol Kolacinski <[email protected]> ... > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c > b/drivers/net/ethernet/intel/ice/ice_ptp.c ... > @@ -2640,6 +2676,16 @@ void ice_ptp_reset(struct ice_pf *pf) > int err, itr = 1; > u64 time_diff; > > + if (ptp->state != ICE_PTP_RESETTING) { > + if (ptp->state == ICE_PTP_READY) { > + ice_ptp_prepare_for_reset(pf); > + } else { > + err = -EINVAL; > + dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n"); > + goto err; > + } > + } nit: perhaps this following is slightly nicer? (completely untested!) if (ptp->state == ICE_PTP_READY) { ice_ptp_prepare_for_reset(pf); } else if (ptp->state != ICE_PTP_RESETTING) { err = -EINVAL; dev_err(ice_pf_to_dev(pf), "PTP was not initialized\n"); goto err; } > + > if (test_bit(ICE_PFR_REQ, pf->state) || > !ice_pf_src_tmr_owned(pf)) > goto pfr; ...
