| Message ID | 20240821121539.374343-8-wojciech.drewek@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for Rx timestamping for both ice and iavf drivers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
From: Wojciech Drewek <wojciech.drewek@intel.com> Date: Wed, 21 Aug 2024 14:15:32 +0200 > From: Jacob Keller <jacob.e.keller@intel.com> > > Implement support for reading the PHC time indirectly via the > VIRTCHNL_OP_1588_PTP_GET_TIME operation. [...] > +/** > + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl > + * @adapter: private adapter structure > + * @cmd: the command structure to send > + * > + * Queue the given command structure into the PTP virtchnl command queue tos > + * end to the PF. > + */ > +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter, > + struct iavf_ptp_aq_cmd *cmd) > +{ > + mutex_lock(&adapter->ptp.aq_cmd_lock); > + list_add_tail(&cmd->list, &adapter->ptp.aq_cmds); > + mutex_unlock(&adapter->ptp.aq_cmd_lock); > + > + adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD; > + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0); Are you sure you need delayed_work here? delayed_work is used only when you need to run it after a delay. If the delay is always 0, then you only need work_struct and queue_work(). > +} > + > +/** > + * iavf_send_phc_read - Send request to read PHC time [...] > +static int iavf_ptp_gettimex64(struct ptp_clock_info *info, > + struct timespec64 *ts, > + struct ptp_system_timestamp *sts) > +{ > + struct iavf_adapter *adapter = iavf_clock_to_adapter(info); > + > + if (!adapter->ptp.initialized) > + return -ENODEV; Why is it -ENODEV here, but -EOPNOTSUPP several functions above, are you sure these codes are the ones expected by the upper layers? > + > + return iavf_read_phc_indirect(adapter, ts, sts); > +} [...] > diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h > index c2ed24cef926..0bb4bddc1495 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h > +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h > @@ -6,9 +6,13 @@ > > #include "iavf_types.h" > > +#define iavf_clock_to_adapter(info) \ > + container_of_const(info, struct iavf_adapter, ptp.info) It's only used in one file, are you sure you need it here in the header? Or it will be used in later patches? [...] > +void iavf_virtchnl_send_ptp_cmd(struct iavf_adapter *adapter) > +{ > + struct device *dev = &adapter->pdev->dev; > + struct iavf_ptp_aq_cmd *cmd; > + int err; > + > + if (!adapter->ptp.initialized) { BTW does it make sense to introduce ptp.initialized since you can always check ptp.clock for being %NULL and it will be the same? > + /* This shouldn't be possible to hit, since no messages should > + * be queued if PTP is not initialized. > + */ > + pci_err(adapter->pdev, "PTP is not initialized\n"); > + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; > + return; > + } > + > + mutex_lock(&adapter->ptp.aq_cmd_lock); > + cmd = list_first_entry_or_null(&adapter->ptp.aq_cmds, > + struct iavf_ptp_aq_cmd, list); > + if (!cmd) { > + /* no further PTP messages to send */ > + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; > + goto out_unlock; > + } > + > + if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) { > + /* bail because we already have a command pending */ > + dev_err(dev, "Cannot send PTP command %d, command %d pending\n", pci_err() > + cmd->v_opcode, adapter->current_op); > + goto out_unlock; > + } > + > + err = iavf_send_pf_msg(adapter, cmd->v_opcode, cmd->msg, cmd->msglen); > + if (!err) { > + /* Command was sent without errors, so we can remove it from > + * the list and discard it. > + */ > + list_del(&cmd->list); > + kfree(cmd); > + } else { > + /* We failed to send the command, try again next cycle */ > + dev_warn(dev, "Failed to send PTP command %d\n", cmd->v_opcode); pci_err() I'd say. > + } > + > + if (list_empty(&adapter->ptp.aq_cmds)) > + /* no further PTP messages to send */ > + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; > + > +out_unlock: > + mutex_unlock(&adapter->ptp.aq_cmd_lock); > +} > + > /** > * iavf_print_link_message - print link up or down > * @adapter: adapter structure > @@ -2093,6 +2151,39 @@ static void iavf_activate_fdir_filters(struct iavf_adapter *adapter) > adapter->aq_required |= IAVF_FLAG_AQ_ADD_FDIR_FILTER; > } > > +/** > + * iavf_virtchnl_ptp_get_time - Respond to VIRTCHNL_OP_1588_PTP_GET_TIME > + * @adapter: private adapter structure > + * @data: the message from the PF > + * @len: length of the message from the PF > + * > + * Handle the VIRTCHNL_OP_1588_PTP_GET_TIME message from the PF. This message > + * is sent by the PF in response to the same op as a request from the VF. > + * Extract the 64bit nanoseconds time from the message and store it in > + * cached_phc_time. Then, notify any thread that is waiting for the update via > + * the wait queue. > + */ > +static void iavf_virtchnl_ptp_get_time(struct iavf_adapter *adapter, > + void *data, u16 len) > +{ > + struct virtchnl_phc_time *msg; > + > + if (len == sizeof(*msg)) { > + msg = (struct virtchnl_phc_time *)data; Redundant cast. > + } else { > + dev_err_once(&adapter->pdev->dev, > + "Invalid VIRTCHNL_OP_1588_PTP_GET_TIME from PF. Got size %u, expected %zu\n", > + len, sizeof(*msg)); > + return; > + } struct virtchnl_phc_time *msg = data; if (len != sizeof(*msg)) // error path adapter->ptp.cached ... IOW there's no point in this complex if-else. > + > + adapter->ptp.cached_phc_time = msg->time; > + adapter->ptp.cached_phc_updated = jiffies; > + adapter->ptp.phc_time_ready = true; > + > + wake_up(&adapter->ptp.phc_time_waitqueue); > +} Thanks, Olek
On 21.08.2024 16:31, Alexander Lobakin wrote: > From: Wojciech Drewek <wojciech.drewek@intel.com> > Date: Wed, 21 Aug 2024 14:15:32 +0200 > >> From: Jacob Keller <jacob.e.keller@intel.com> >> >> Implement support for reading the PHC time indirectly via the >> VIRTCHNL_OP_1588_PTP_GET_TIME operation. > > [...] > >> +/** >> + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl >> + * @adapter: private adapter structure >> + * @cmd: the command structure to send >> + * >> + * Queue the given command structure into the PTP virtchnl command queue tos >> + * end to the PF. >> + */ >> +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter, >> + struct iavf_ptp_aq_cmd *cmd) >> +{ >> + mutex_lock(&adapter->ptp.aq_cmd_lock); >> + list_add_tail(&cmd->list, &adapter->ptp.aq_cmds); >> + mutex_unlock(&adapter->ptp.aq_cmd_lock); >> + >> + adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD; >> + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0); > > Are you sure you need delayed_work here? delayed_work is used only when > you need to run it after a delay. If the delay is always 0, then you > only need work_struct and queue_work(). I think that Jake's intention here was to execute the work that is already queued, not to queue new work > >> +} >> + >> +/** >> + * iavf_send_phc_read - Send request to read PHC time > > [...] > >> +static int iavf_ptp_gettimex64(struct ptp_clock_info *info, >> + struct timespec64 *ts, >> + struct ptp_system_timestamp *sts) >> +{ >> + struct iavf_adapter *adapter = iavf_clock_to_adapter(info); >> + >> + if (!adapter->ptp.initialized) >> + return -ENODEV; > > Why is it -ENODEV here, but -EOPNOTSUPP several functions above, are you > sure these codes are the ones expected by the upper layers? I'll use ENODEV in both cases > >> + >> + return iavf_read_phc_indirect(adapter, ts, sts); >> +} > > [...] > >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> index c2ed24cef926..0bb4bddc1495 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> @@ -6,9 +6,13 @@ >> >> #include "iavf_types.h" >> >> +#define iavf_clock_to_adapter(info) \ >> + container_of_const(info, struct iavf_adapter, ptp.info) > > It's only used in one file, are you sure you need it here in the header? > Or it will be used in later patches? I can define it in iavf_ptp.c if you want > > [...] > >> +void iavf_virtchnl_send_ptp_cmd(struct iavf_adapter *adapter) >> +{ >> + struct device *dev = &adapter->pdev->dev; >> + struct iavf_ptp_aq_cmd *cmd; >> + int err; >> + >> + if (!adapter->ptp.initialized) { > > BTW does it make sense to introduce ptp.initialized since you can always > check ptp.clock for being %NULL and it will be the same? I'll think about it > >> + /* This shouldn't be possible to hit, since no messages should >> + * be queued if PTP is not initialized. >> + */ >> + pci_err(adapter->pdev, "PTP is not initialized\n"); >> + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; >> + return; >> + } >> + >> + mutex_lock(&adapter->ptp.aq_cmd_lock); >> + cmd = list_first_entry_or_null(&adapter->ptp.aq_cmds, >> + struct iavf_ptp_aq_cmd, list); >> + if (!cmd) { >> + /* no further PTP messages to send */ >> + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; >> + goto out_unlock; >> + } >> + >> + if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) { >> + /* bail because we already have a command pending */ >> + dev_err(dev, "Cannot send PTP command %d, command %d pending\n", > > pci_err() sure > >> + cmd->v_opcode, adapter->current_op); >> + goto out_unlock; >> + } >> + >> + err = iavf_send_pf_msg(adapter, cmd->v_opcode, cmd->msg, cmd->msglen); >> + if (!err) { >> + /* Command was sent without errors, so we can remove it from >> + * the list and discard it. >> + */ >> + list_del(&cmd->list); >> + kfree(cmd); >> + } else { >> + /* We failed to send the command, try again next cycle */ >> + dev_warn(dev, "Failed to send PTP command %d\n", cmd->v_opcode); > > pci_err() I'd say. sure > >> + } >> + >> + if (list_empty(&adapter->ptp.aq_cmds)) >> + /* no further PTP messages to send */ >> + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; >> + >> +out_unlock: >> + mutex_unlock(&adapter->ptp.aq_cmd_lock); >> +} >> + >> /** >> * iavf_print_link_message - print link up or down >> * @adapter: adapter structure >> @@ -2093,6 +2151,39 @@ static void iavf_activate_fdir_filters(struct iavf_adapter *adapter) >> adapter->aq_required |= IAVF_FLAG_AQ_ADD_FDIR_FILTER; >> } >> >> +/** >> + * iavf_virtchnl_ptp_get_time - Respond to VIRTCHNL_OP_1588_PTP_GET_TIME >> + * @adapter: private adapter structure >> + * @data: the message from the PF >> + * @len: length of the message from the PF >> + * >> + * Handle the VIRTCHNL_OP_1588_PTP_GET_TIME message from the PF. This message >> + * is sent by the PF in response to the same op as a request from the VF. >> + * Extract the 64bit nanoseconds time from the message and store it in >> + * cached_phc_time. Then, notify any thread that is waiting for the update via >> + * the wait queue. >> + */ >> +static void iavf_virtchnl_ptp_get_time(struct iavf_adapter *adapter, >> + void *data, u16 len) >> +{ >> + struct virtchnl_phc_time *msg; >> + >> + if (len == sizeof(*msg)) { >> + msg = (struct virtchnl_phc_time *)data; > > Redundant cast. yup > >> + } else { >> + dev_err_once(&adapter->pdev->dev, >> + "Invalid VIRTCHNL_OP_1588_PTP_GET_TIME from PF. Got size %u, expected %zu\n", >> + len, sizeof(*msg)); >> + return; >> + } > > struct virtchnl_phc_time *msg = data; > > if (len != sizeof(*msg)) > // error path > > adapter->ptp.cached ... > > IOW there's no point in this complex if-else. Agree > >> + >> + adapter->ptp.cached_phc_time = msg->time; >> + adapter->ptp.cached_phc_updated = jiffies; >> + adapter->ptp.phc_time_ready = true; >> + >> + wake_up(&adapter->ptp.phc_time_waitqueue); >> +} > > Thanks, > Olek
From: Wojciech Drewek <wojciech.drewek@intel.com> Date: Wed, 28 Aug 2024 13:15:09 +0200 > > > On 21.08.2024 16:31, Alexander Lobakin wrote: >> From: Wojciech Drewek <wojciech.drewek@intel.com> >> Date: Wed, 21 Aug 2024 14:15:32 +0200 >> >>> From: Jacob Keller <jacob.e.keller@intel.com> >>> >>> Implement support for reading the PHC time indirectly via the >>> VIRTCHNL_OP_1588_PTP_GET_TIME operation. >> >> [...] >> >>> +/** >>> + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl >>> + * @adapter: private adapter structure >>> + * @cmd: the command structure to send >>> + * >>> + * Queue the given command structure into the PTP virtchnl command queue tos >>> + * end to the PF. >>> + */ >>> +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter, >>> + struct iavf_ptp_aq_cmd *cmd) >>> +{ >>> + mutex_lock(&adapter->ptp.aq_cmd_lock); >>> + list_add_tail(&cmd->list, &adapter->ptp.aq_cmds); >>> + mutex_unlock(&adapter->ptp.aq_cmd_lock); >>> + >>> + adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD; >>> + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0); >> >> Are you sure you need delayed_work here? delayed_work is used only when >> you need to run it after a delay. If the delay is always 0, then you >> only need work_struct and queue_work(). > > I think that Jake's intention here was to execute the work that is already queued, > not to queue new work mod_delayed_work(0) works exactly as queue_work(), which is: * if the work is already queued and the timeout is non-zero, modify the timeout * if the work is already queued and the timeout is zero, do nothing * if the work is not queued, queue it So my comment it still valid. You don't need delayed_work, but work_struct. > >> >>> +} >>> + >>> +/** >>> + * iavf_send_phc_read - Send request to read PHC time >> >> [...] >> >>> +static int iavf_ptp_gettimex64(struct ptp_clock_info *info, >>> + struct timespec64 *ts, >>> + struct ptp_system_timestamp *sts) >>> +{ >>> + struct iavf_adapter *adapter = iavf_clock_to_adapter(info); >>> + >>> + if (!adapter->ptp.initialized) >>> + return -ENODEV; >> >> Why is it -ENODEV here, but -EOPNOTSUPP several functions above, are you >> sure these codes are the ones expected by the upper layers? > > I'll use ENODEV in both cases But why -ENODEV? Can you show me some other drivers and/or core PTP code which use it? > >> >>> + >>> + return iavf_read_phc_indirect(adapter, ts, sts); >>> +} Thanks, Olek
On 28.08.2024 14:05, Alexander Lobakin wrote: > From: Wojciech Drewek <wojciech.drewek@intel.com> > Date: Wed, 28 Aug 2024 13:15:09 +0200 > >> >> >> On 21.08.2024 16:31, Alexander Lobakin wrote: >>> From: Wojciech Drewek <wojciech.drewek@intel.com> >>> Date: Wed, 21 Aug 2024 14:15:32 +0200 >>> >>>> From: Jacob Keller <jacob.e.keller@intel.com> >>>> >>>> Implement support for reading the PHC time indirectly via the >>>> VIRTCHNL_OP_1588_PTP_GET_TIME operation. >>> >>> [...] >>> >>>> +/** >>>> + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl >>>> + * @adapter: private adapter structure >>>> + * @cmd: the command structure to send >>>> + * >>>> + * Queue the given command structure into the PTP virtchnl command queue tos >>>> + * end to the PF. >>>> + */ >>>> +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter, >>>> + struct iavf_ptp_aq_cmd *cmd) >>>> +{ >>>> + mutex_lock(&adapter->ptp.aq_cmd_lock); >>>> + list_add_tail(&cmd->list, &adapter->ptp.aq_cmds); >>>> + mutex_unlock(&adapter->ptp.aq_cmd_lock); >>>> + >>>> + adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD; >>>> + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0); >>> >>> Are you sure you need delayed_work here? delayed_work is used only when >>> you need to run it after a delay. If the delay is always 0, then you >>> only need work_struct and queue_work(). >> >> I think that Jake's intention here was to execute the work that is already queued, >> not to queue new work > > mod_delayed_work(0) works exactly as queue_work(), which is: > > * if the work is already queued and the timeout is non-zero, modify the > timeout > * if the work is already queued and the timeout is zero, do nothing > * if the work is not queued, queue it > > So my comment it still valid. You don't need delayed_work, but work_struct. Okay, thx for explanation > >> >>> >>>> +} >>>> + >>>> +/** >>>> + * iavf_send_phc_read - Send request to read PHC time >>> >>> [...] >>> >>>> +static int iavf_ptp_gettimex64(struct ptp_clock_info *info, >>>> + struct timespec64 *ts, >>>> + struct ptp_system_timestamp *sts) >>>> +{ >>>> + struct iavf_adapter *adapter = iavf_clock_to_adapter(info); >>>> + >>>> + if (!adapter->ptp.initialized) >>>> + return -ENODEV; >>> >>> Why is it -ENODEV here, but -EOPNOTSUPP several functions above, are you >>> sure these codes are the ones expected by the upper layers? >> >> I'll use ENODEV in both cases > > But why -ENODEV? Can you show me some other drivers and/or core PTP code > which use it? I couldn't find many examples, since @initialized is set to false mainly when PTP is not supported than I think EOPNOTSUPP would be a better pick. > >> >>> >>>> + >>>> + return iavf_read_phc_indirect(adapter, ts, sts); >>>> +} > > Thanks, > Olek
On 8/21/2024 4:31 PM, Alexander Lobakin wrote: > From: Wojciech Drewek <wojciech.drewek@intel.com> > Date: Wed, 21 Aug 2024 14:15:32 +0200 > >> From: Jacob Keller <jacob.e.keller@intel.com> >> >> Implement support for reading the PHC time indirectly via the >> VIRTCHNL_OP_1588_PTP_GET_TIME operation. > > [...] > >> +/** >> + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl >> + * @adapter: private adapter structure >> + * @cmd: the command structure to send >> + * >> + * Queue the given command structure into the PTP virtchnl command queue tos >> + * end to the PF. >> + */ >> +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter, >> + struct iavf_ptp_aq_cmd *cmd) >> +{ >> + mutex_lock(&adapter->ptp.aq_cmd_lock); >> + list_add_tail(&cmd->list, &adapter->ptp.aq_cmds); >> + mutex_unlock(&adapter->ptp.aq_cmd_lock); >> + >> + adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD; >> + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0); > > Are you sure you need delayed_work here? delayed_work is used only when > you need to run it after a delay. If the delay is always 0, then you > only need work_struct and queue_work(). > Sorry for late response but I was on quite long vacation. Regarding your question - I think it is needed to have mod_delayed_work() here, at least as for now. I agree with your suggestion to use queue_work() but this function takes as parameter work_struct and in our case the adapter->watchdog_task field is of struct delayed_work type. It uses the function iavf_watchdog_task() which does plenty of things (including sending ptp cmd). Changing adapter->watchdog_task to be just struct work_struct is not applicable here as in iavf_main.c file in few places we add it to queue with different time. Make long story short - I agree with your argument but as far as this 0 delay is not causing performance regression then I will stick with this solution implemented by Jake. >> +} >> + >> +/** >> + * iavf_send_phc_read - Send request to read PHC time > > [...] > >> +static int iavf_ptp_gettimex64(struct ptp_clock_info *info, >> + struct timespec64 *ts, >> + struct ptp_system_timestamp *sts) >> +{ >> + struct iavf_adapter *adapter = iavf_clock_to_adapter(info); >> + >> + if (!adapter->ptp.initialized) >> + return -ENODEV; > > Why is it -ENODEV here, but -EOPNOTSUPP several functions above, are you > sure these codes are the ones expected by the upper layers? > >> + >> + return iavf_read_phc_indirect(adapter, ts, sts); >> +} > > [...] > >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> index c2ed24cef926..0bb4bddc1495 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> @@ -6,9 +6,13 @@ >> >> #include "iavf_types.h" >> >> +#define iavf_clock_to_adapter(info) \ >> + container_of_const(info, struct iavf_adapter, ptp.info) > > It's only used in one file, are you sure you need it here in the header? > Or it will be used in later patches? > > [...] > >> +void iavf_virtchnl_send_ptp_cmd(struct iavf_adapter *adapter) >> +{ >> + struct device *dev = &adapter->pdev->dev; >> + struct iavf_ptp_aq_cmd *cmd; >> + int err; >> + >> + if (!adapter->ptp.initialized) { > > BTW does it make sense to introduce ptp.initialized since you can always > check ptp.clock for being %NULL and it will be the same? > >> + /* This shouldn't be possible to hit, since no messages should >> + * be queued if PTP is not initialized. >> + */ >> + pci_err(adapter->pdev, "PTP is not initialized\n"); >> + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; >> + return; >> + } >> + >> + mutex_lock(&adapter->ptp.aq_cmd_lock); >> + cmd = list_first_entry_or_null(&adapter->ptp.aq_cmds, >> + struct iavf_ptp_aq_cmd, list); >> + if (!cmd) { >> + /* no further PTP messages to send */ >> + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; >> + goto out_unlock; >> + } >> + >> + if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) { >> + /* bail because we already have a command pending */ >> + dev_err(dev, "Cannot send PTP command %d, command %d pending\n", > > pci_err() > >> + cmd->v_opcode, adapter->current_op); >> + goto out_unlock; >> + } >> + >> + err = iavf_send_pf_msg(adapter, cmd->v_opcode, cmd->msg, cmd->msglen); >> + if (!err) { >> + /* Command was sent without errors, so we can remove it from >> + * the list and discard it. >> + */ >> + list_del(&cmd->list); >> + kfree(cmd); >> + } else { >> + /* We failed to send the command, try again next cycle */ >> + dev_warn(dev, "Failed to send PTP command %d\n", cmd->v_opcode); > > pci_err() I'd say. > >> + } >> + >> + if (list_empty(&adapter->ptp.aq_cmds)) >> + /* no further PTP messages to send */ >> + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; >> + >> +out_unlock: >> + mutex_unlock(&adapter->ptp.aq_cmd_lock); >> +} >> + >> /** >> * iavf_print_link_message - print link up or down >> * @adapter: adapter structure >> @@ -2093,6 +2151,39 @@ static void iavf_activate_fdir_filters(struct iavf_adapter *adapter) >> adapter->aq_required |= IAVF_FLAG_AQ_ADD_FDIR_FILTER; >> } >> >> +/** >> + * iavf_virtchnl_ptp_get_time - Respond to VIRTCHNL_OP_1588_PTP_GET_TIME >> + * @adapter: private adapter structure >> + * @data: the message from the PF >> + * @len: length of the message from the PF >> + * >> + * Handle the VIRTCHNL_OP_1588_PTP_GET_TIME message from the PF. This message >> + * is sent by the PF in response to the same op as a request from the VF. >> + * Extract the 64bit nanoseconds time from the message and store it in >> + * cached_phc_time. Then, notify any thread that is waiting for the update via >> + * the wait queue. >> + */ >> +static void iavf_virtchnl_ptp_get_time(struct iavf_adapter *adapter, >> + void *data, u16 len) >> +{ >> + struct virtchnl_phc_time *msg; >> + >> + if (len == sizeof(*msg)) { >> + msg = (struct virtchnl_phc_time *)data; > > Redundant cast. > >> + } else { >> + dev_err_once(&adapter->pdev->dev, >> + "Invalid VIRTCHNL_OP_1588_PTP_GET_TIME from PF. Got size %u, expected %zu\n", >> + len, sizeof(*msg)); >> + return; >> + } > > struct virtchnl_phc_time *msg = data; > > if (len != sizeof(*msg)) > // error path > > adapter->ptp.cached ... > > IOW there's no point in this complex if-else. > >> + >> + adapter->ptp.cached_phc_time = msg->time; >> + adapter->ptp.cached_phc_updated = jiffies; >> + adapter->ptp.phc_time_ready = true; >> + >> + wake_up(&adapter->ptp.phc_time_waitqueue); >> +} > > Thanks, > Olek >
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> Date: Tue, 1 Oct 2024 09:20:14 +0200 > > > On 8/21/2024 4:31 PM, Alexander Lobakin wrote: >> From: Wojciech Drewek <wojciech.drewek@intel.com> >> Date: Wed, 21 Aug 2024 14:15:32 +0200 >> >>> From: Jacob Keller <jacob.e.keller@intel.com> >>> >>> Implement support for reading the PHC time indirectly via the >>> VIRTCHNL_OP_1588_PTP_GET_TIME operation. >> >> [...] >> >>> +/** >>> + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl >>> + * @adapter: private adapter structure >>> + * @cmd: the command structure to send >>> + * >>> + * Queue the given command structure into the PTP virtchnl command >>> queue tos >>> + * end to the PF. >>> + */ >>> +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter, >>> + struct iavf_ptp_aq_cmd *cmd) >>> +{ >>> + mutex_lock(&adapter->ptp.aq_cmd_lock); >>> + list_add_tail(&cmd->list, &adapter->ptp.aq_cmds); >>> + mutex_unlock(&adapter->ptp.aq_cmd_lock); >>> + >>> + adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD; >>> + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0); >> >> Are you sure you need delayed_work here? delayed_work is used only when >> you need to run it after a delay. If the delay is always 0, then you >> only need work_struct and queue_work(). >> > > Sorry for late response but I was on quite long vacation. > > Regarding your question - I think it is needed to have > mod_delayed_work() here, at least as for now. I agree with your > suggestion to use queue_work() but this function takes as parameter > work_struct and in our case the adapter->watchdog_task field is of > struct delayed_work type. It uses the function iavf_watchdog_task() > which does plenty of things (including sending ptp cmd). Changing > adapter->watchdog_task to be just struct work_struct is not applicable > here as in iavf_main.c file in few places we add it to queue with > different time. Aaaah I'm sorry I didn't notice that watchdog_task is used in other placed, not only here. Ack, leave it as it is. > > Make long story short - I agree with your argument but as far as this > 0 delay is not causing performance regression then I will stick with > this solution implemented by Jake. Thanks, Olek
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c index 8a99aab5bf6f..2d862a27d7a5 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_main.c +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c @@ -2235,7 +2235,10 @@ static int iavf_process_aq_command(struct iavf_adapter *adapter) iavf_enable_vlan_insertion_v2(adapter, ETH_P_8021AD); return 0; } - + if (adapter->aq_required & IAVF_FLAG_AQ_SEND_PTP_CMD) { + iavf_virtchnl_send_ptp_cmd(adapter); + return IAVF_SUCCESS; + } if (adapter->aq_required & IAVF_FLAG_AQ_REQUEST_STATS) { iavf_request_stats(adapter); return 0; @@ -5326,6 +5329,10 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) /* Setup the wait queue for indicating virtchannel events */ init_waitqueue_head(&adapter->vc_waitqueue); + INIT_LIST_HEAD(&adapter->ptp.aq_cmds); + init_waitqueue_head(&adapter->ptp.phc_time_waitqueue); + mutex_init(&adapter->ptp.aq_cmd_lock); + queue_delayed_work(adapter->wq, &adapter->watchdog_task, msecs_to_jiffies(5 * (pdev->devfn & 0x07))); /* Initialization goes on in the work. Do not add more of it below. */ diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.c b/drivers/net/ethernet/intel/iavf/iavf_ptp.c index 045aa8690ac2..d709d381958f 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.c +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c @@ -21,6 +21,138 @@ bool iavf_ptp_cap_supported(const struct iavf_adapter *adapter, u32 cap) return (adapter->ptp.hw_caps.caps & cap) == cap; } +/** + * iavf_allocate_ptp_cmd - Allocate a PTP command message structure + * @v_opcode: the virtchnl opcode + * @msglen: length in bytes of the associated virtchnl structure + * + * Allocates a PTP command message and pre-fills it with the provided message + * length and opcode. + * + * Return: allocated PTP command. + */ +static struct iavf_ptp_aq_cmd *iavf_allocate_ptp_cmd(enum virtchnl_ops v_opcode, + u16 msglen) +{ + struct iavf_ptp_aq_cmd *cmd; + + cmd = kzalloc(struct_size(cmd, msg, msglen), GFP_KERNEL); + if (!cmd) + return NULL; + + cmd->v_opcode = v_opcode; + cmd->msglen = msglen; + + return cmd; +} + +/** + * iavf_queue_ptp_cmd - Queue PTP command for sending over virtchnl + * @adapter: private adapter structure + * @cmd: the command structure to send + * + * Queue the given command structure into the PTP virtchnl command queue tos + * end to the PF. + */ +static void iavf_queue_ptp_cmd(struct iavf_adapter *adapter, + struct iavf_ptp_aq_cmd *cmd) +{ + mutex_lock(&adapter->ptp.aq_cmd_lock); + list_add_tail(&cmd->list, &adapter->ptp.aq_cmds); + mutex_unlock(&adapter->ptp.aq_cmd_lock); + + adapter->aq_required |= IAVF_FLAG_AQ_SEND_PTP_CMD; + mod_delayed_work(adapter->wq, &adapter->watchdog_task, 0); +} + +/** + * iavf_send_phc_read - Send request to read PHC time + * @adapter: private adapter structure + * + * Send a request to obtain the PTP hardware clock time. This allocates the + * VIRTCHNL_OP_1588_PTP_GET_TIME message and queues it up to send to + * indirectly read the PHC time. + * + * This function does not wait for the reply from the PF. + * + * Return: 0 if success, error code otherwise. + */ +static int iavf_send_phc_read(struct iavf_adapter *adapter) +{ + struct iavf_ptp_aq_cmd *cmd; + + if (!adapter->ptp.initialized) + return -EOPNOTSUPP; + + cmd = iavf_allocate_ptp_cmd(VIRTCHNL_OP_1588_PTP_GET_TIME, + sizeof(struct virtchnl_phc_time)); + if (!cmd) + return -ENOMEM; + + iavf_queue_ptp_cmd(adapter, cmd); + + return 0; +} + +/** + * iavf_read_phc_indirect - Indirectly read the PHC time via virtchnl + * @adapter: private adapter structure + * @ts: storage for the timestamp value + * @sts: system timestamp values before and after the read + * + * Used when the device does not have direct register access to the PHC time. + * Indirectly reads the time via the VIRTCHNL_OP_1588_PTP_GET_TIME, and waits + * for the reply from the PF. + * + * Based on some simple measurements using ftrace and phc2sys, this clock + * access method has about a ~110 usec latency even when the system is not + * under load. In order to achieve acceptable results when using phc2sys with + * the indirect clock access method, it is recommended to use more + * conservative proportional and integration constants with the P/I servo. + * + * Return: 0 if success, error code otherwise. + */ +static int iavf_read_phc_indirect(struct iavf_adapter *adapter, + struct timespec64 *ts, + struct ptp_system_timestamp *sts) +{ + long ret; + int err; + + adapter->ptp.phc_time_ready = false; + ptp_read_system_prets(sts); + + err = iavf_send_phc_read(adapter); + if (err) + return err; + + ret = wait_event_interruptible_timeout(adapter->ptp.phc_time_waitqueue, + adapter->ptp.phc_time_ready, + HZ); + if (ret < 0) + return ret; + else if (!ret) + return -EBUSY; + + *ts = ns_to_timespec64(adapter->ptp.cached_phc_time); + + ptp_read_system_postts(sts); + + return 0; +} + +static int iavf_ptp_gettimex64(struct ptp_clock_info *info, + struct timespec64 *ts, + struct ptp_system_timestamp *sts) +{ + struct iavf_adapter *adapter = iavf_clock_to_adapter(info); + + if (!adapter->ptp.initialized) + return -ENODEV; + + return iavf_read_phc_indirect(adapter, ts, sts); +} + /** * iavf_ptp_register_clock - Register a new PTP for userspace * @adapter: private adapter structure @@ -39,6 +171,7 @@ static int iavf_ptp_register_clock(struct iavf_adapter *adapter) snprintf(ptp_info->name, sizeof(ptp_info->name), "%s-%s-clk", dev_driver_string(dev), dev_name(dev)); ptp_info->owner = THIS_MODULE; + ptp_info->gettimex64 = iavf_ptp_gettimex64; adapter->ptp.clock = ptp_clock_register(ptp_info, dev); if (IS_ERR(adapter->ptp.clock)) { @@ -89,6 +222,8 @@ void iavf_ptp_init(struct iavf_adapter *adapter) */ void iavf_ptp_release(struct iavf_adapter *adapter) { + struct iavf_ptp_aq_cmd *cmd, *tmp; + adapter->ptp.initialized = false; if (!IS_ERR_OR_NULL(adapter->ptp.clock)) { @@ -97,6 +232,15 @@ void iavf_ptp_release(struct iavf_adapter *adapter) ptp_clock_unregister(adapter->ptp.clock); adapter->ptp.clock = NULL; } + + /* Cancel any remaining uncompleted PTP clock commands */ + mutex_lock(&adapter->ptp.aq_cmd_lock); + list_for_each_entry_safe(cmd, tmp, &adapter->ptp.aq_cmds, list) { + list_del(&cmd->list); + kfree(cmd); + } + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; + mutex_unlock(&adapter->ptp.aq_cmd_lock); } /** diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h index c2ed24cef926..0bb4bddc1495 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h @@ -6,9 +6,13 @@ #include "iavf_types.h" +#define iavf_clock_to_adapter(info) \ + container_of_const(info, struct iavf_adapter, ptp.info) + void iavf_ptp_init(struct iavf_adapter *adapter); void iavf_ptp_release(struct iavf_adapter *adapter); void iavf_ptp_process_caps(struct iavf_adapter *adapter); bool iavf_ptp_cap_supported(const struct iavf_adapter *adapter, u32 cap); +void iavf_virtchnl_send_ptp_cmd(struct iavf_adapter *adapter); #endif /* _IAVF_PTP_H_ */ diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c index 03e880d756de..89eb296128a9 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c @@ -1522,6 +1522,64 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid) VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2); } +/** + * iavf_virtchnl_send_ptp_cmd - Send one queued PTP command + * @adapter: adapter private structure + * + * De-queue one PTP command request and send the command message to the PF. + * Clear IAVF_FLAG_AQ_SEND_PTP_CMD if no more messages are left to send. + */ +void iavf_virtchnl_send_ptp_cmd(struct iavf_adapter *adapter) +{ + struct device *dev = &adapter->pdev->dev; + struct iavf_ptp_aq_cmd *cmd; + int err; + + if (!adapter->ptp.initialized) { + /* This shouldn't be possible to hit, since no messages should + * be queued if PTP is not initialized. + */ + pci_err(adapter->pdev, "PTP is not initialized\n"); + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; + return; + } + + mutex_lock(&adapter->ptp.aq_cmd_lock); + cmd = list_first_entry_or_null(&adapter->ptp.aq_cmds, + struct iavf_ptp_aq_cmd, list); + if (!cmd) { + /* no further PTP messages to send */ + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; + goto out_unlock; + } + + if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) { + /* bail because we already have a command pending */ + dev_err(dev, "Cannot send PTP command %d, command %d pending\n", + cmd->v_opcode, adapter->current_op); + goto out_unlock; + } + + err = iavf_send_pf_msg(adapter, cmd->v_opcode, cmd->msg, cmd->msglen); + if (!err) { + /* Command was sent without errors, so we can remove it from + * the list and discard it. + */ + list_del(&cmd->list); + kfree(cmd); + } else { + /* We failed to send the command, try again next cycle */ + dev_warn(dev, "Failed to send PTP command %d\n", cmd->v_opcode); + } + + if (list_empty(&adapter->ptp.aq_cmds)) + /* no further PTP messages to send */ + adapter->aq_required &= ~IAVF_FLAG_AQ_SEND_PTP_CMD; + +out_unlock: + mutex_unlock(&adapter->ptp.aq_cmd_lock); +} + /** * iavf_print_link_message - print link up or down * @adapter: adapter structure @@ -2093,6 +2151,39 @@ static void iavf_activate_fdir_filters(struct iavf_adapter *adapter) adapter->aq_required |= IAVF_FLAG_AQ_ADD_FDIR_FILTER; } +/** + * iavf_virtchnl_ptp_get_time - Respond to VIRTCHNL_OP_1588_PTP_GET_TIME + * @adapter: private adapter structure + * @data: the message from the PF + * @len: length of the message from the PF + * + * Handle the VIRTCHNL_OP_1588_PTP_GET_TIME message from the PF. This message + * is sent by the PF in response to the same op as a request from the VF. + * Extract the 64bit nanoseconds time from the message and store it in + * cached_phc_time. Then, notify any thread that is waiting for the update via + * the wait queue. + */ +static void iavf_virtchnl_ptp_get_time(struct iavf_adapter *adapter, + void *data, u16 len) +{ + struct virtchnl_phc_time *msg; + + if (len == sizeof(*msg)) { + msg = (struct virtchnl_phc_time *)data; + } else { + dev_err_once(&adapter->pdev->dev, + "Invalid VIRTCHNL_OP_1588_PTP_GET_TIME from PF. Got size %u, expected %zu\n", + len, sizeof(*msg)); + return; + } + + adapter->ptp.cached_phc_time = msg->time; + adapter->ptp.cached_phc_updated = jiffies; + adapter->ptp.phc_time_ready = true; + + wake_up(&adapter->ptp.phc_time_waitqueue); +} + /** * iavf_virtchnl_completion * @adapter: adapter structure @@ -2503,6 +2594,9 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter, /* process any state change needed due to new capabilities */ iavf_ptp_process_caps(adapter); break; + case VIRTCHNL_OP_1588_PTP_GET_TIME: + iavf_virtchnl_ptp_get_time(adapter, msg, msglen); + break; case VIRTCHNL_OP_ENABLE_QUEUES: /* enable transmits */ iavf_irq_enable(adapter, true);