| Message ID | 20240226-feature_ptp_netnext-v9-7-455611549f21@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Make timestamping selectable | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| netdev/apply | fail | Patch does not apply to net-next |
On Mon, 26 Feb 2024 14:39:58 +0100 Kory Maincent wrote:
> +static DEFINE_XARRAY_FLAGS(ptp_clocks_map, XA_FLAGS_LOCK_IRQ | XA_FLAGS_ALLOC);
Why _IRQ? anything on the fastpath hopefully has a pointer to the clock
already, I'd hope. And we often reserve ID 0 as invalid.
IOW DEFINE_XARRAY_ALLOC1() ?
BTW could be a standalone patch, Xarray conversion from IDA is an
improvement in itself.
On Mon, 4 Mar 2024 18:47:37 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Mon, 26 Feb 2024 14:39:58 +0100 Kory Maincent wrote: > > +static DEFINE_XARRAY_FLAGS(ptp_clocks_map, XA_FLAGS_LOCK_IRQ | > > XA_FLAGS_ALLOC); > > Why _IRQ? anything on the fastpath hopefully has a pointer to the clock > already, I'd hope. And we often reserve ID 0 as invalid. To keep the same flag as IDA_INIT_FLAGS, I am not expert in xarray so I just keep it without questioning it. Do you think I should remove it? ID 0 was valid for phc. IMHO makes it invalid is not a good idea, it will change the phc id value for current board on the field. > > BTW could be a standalone patch, Xarray conversion from IDA is an > improvement in itself. Indeed. Do you prefer this patch to be standalone? Regards,
On Tue, 5 Mar 2024 10:02:59 +0100 Köry Maincent wrote: > On Mon, 4 Mar 2024 18:47:37 -0800 > Jakub Kicinski <kuba@kernel.org> wrote: > > > On Mon, 26 Feb 2024 14:39:58 +0100 Kory Maincent wrote: > > > +static DEFINE_XARRAY_FLAGS(ptp_clocks_map, XA_FLAGS_LOCK_IRQ | > > > XA_FLAGS_ALLOC); > > > > Why _IRQ? anything on the fastpath hopefully has a pointer to the clock > > already, I'd hope. And we often reserve ID 0 as invalid. > > To keep the same flag as IDA_INIT_FLAGS, I am not expert in xarray so I just > keep it without questioning it. Do you think I should remove it? Yes, I believe those defaults are just "to be safe". > ID 0 was valid for phc. IMHO makes it invalid is not a good idea, it > will change the phc id value for current board on the field. Ah, right, let's keep it then. We'll have to use -1 as invalid. > > BTW could be a standalone patch, Xarray conversion from IDA is an > > improvement in itself. > > Indeed. Do you prefer this patch to be standalone? May be a personal preference but I do feel like sending general improvements separately from large new features makes the process more smooth.
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 3aaf1a3430c5..f374b1e89780 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -31,7 +31,7 @@ struct class *ptp_class; static dev_t ptp_devt; -static DEFINE_IDA(ptp_clocks_map); +static DEFINE_XARRAY_FLAGS(ptp_clocks_map, XA_FLAGS_LOCK_IRQ | XA_FLAGS_ALLOC); /* time stamp event queue operations */ @@ -201,7 +201,7 @@ static void ptp_clock_release(struct device *dev) bitmap_free(tsevq->mask); kfree(tsevq); debugfs_remove(ptp->debugfs_root); - ida_free(&ptp_clocks_map, ptp->index); + xa_erase(&ptp_clocks_map, ptp->index); kfree(ptp); } @@ -246,11 +246,10 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, if (ptp == NULL) goto no_memory; - index = ida_alloc_max(&ptp_clocks_map, MINORMASK, GFP_KERNEL); - if (index < 0) { - err = index; + err = xa_alloc(&ptp_clocks_map, &index, ptp, xa_limit_31b, + GFP_KERNEL); + if (err) goto no_slot; - } ptp->clock.ops = ptp_clock_ops; ptp->info = info; @@ -378,7 +377,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, list_del(&queue->qlist); kfree(queue); no_memory_queue: - ida_free(&ptp_clocks_map, index); + xa_erase(&ptp_clocks_map, index); no_slot: kfree(ptp); no_memory: @@ -511,7 +510,7 @@ static void __exit ptp_exit(void) { class_destroy(ptp_class); unregister_chrdev_region(ptp_devt, MINORMASK + 1); - ida_destroy(&ptp_clocks_map); + xa_destroy(&ptp_clocks_map); } static int __init ptp_init(void)
Move from simple ida to xarray for storing and loading the ptp_clock pointer. This prepares support for future hardware timestamp selection by being able to link the ptp clock index to its pointer. Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> --- Changes in v8: - New patch --- drivers/ptp/ptp_clock.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)