| Message ID | 20201210142908.169597-4-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify I/O port and MMIO trap handling | expand |
Hi Andre, On 12/10/20 2:28 PM, Andre Przywara wrote: > The ioport routines support a special way of registering FDT node > generator functions. There is no reason to have this separate from the > already existing way via the device header. > > Now that the only user of this special ioport variety has been > transferred, we can retire this code, to simplify ioport handling. One comment below, but otherwise very nice cleanup. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > include/kvm/ioport.h | 4 ---- > ioport.c | 34 ---------------------------------- > 2 files changed, 38 deletions(-) > > diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h > index d0213541..a61038e2 100644 > --- a/include/kvm/ioport.h > +++ b/include/kvm/ioport.h > @@ -29,10 +29,6 @@ struct ioport { > struct ioport_operations { > bool (*io_in)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); > bool (*io_out)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); > - void (*generate_fdt_node)(struct ioport *ioport, void *fdt, > - void (*generate_irq_prop)(void *fdt, > - u8 irq, > - enum irq_type)); > }; > > void ioport__map_irq(u8 *irq); > diff --git a/ioport.c b/ioport.c > index 667e8386..b98836d3 100644 > --- a/ioport.c > +++ b/ioport.c > @@ -56,7 +56,6 @@ static struct ioport *ioport_get(struct rb_root *root, u64 addr) > /* Called with ioport_lock held. */ > static void ioport_unregister(struct rb_root *root, struct ioport *data) > { > - device__unregister(&data->dev_hdr); > ioport_remove(root, data); > free(data); > } > @@ -70,30 +69,6 @@ static void ioport_put(struct rb_root *root, struct ioport *data) > mutex_unlock(&ioport_lock); > } > > -#ifdef CONFIG_HAS_LIBFDT > -static void generate_ioport_fdt_node(void *fdt, > - struct device_header *dev_hdr, > - void (*generate_irq_prop)(void *fdt, > - u8 irq, > - enum irq_type)) > -{ > - struct ioport *ioport = container_of(dev_hdr, struct ioport, dev_hdr); > - struct ioport_operations *ops = ioport->ops; > - > - if (ops->generate_fdt_node) > - ops->generate_fdt_node(ioport, fdt, generate_irq_prop); > -} > -#else > -static void generate_ioport_fdt_node(void *fdt, > - struct device_header *dev_hdr, > - void (*generate_irq_prop)(void *fdt, > - u8 irq, > - enum irq_type)) > -{ > - die("Unable to generate device tree nodes without libfdt\n"); > -} > -#endif > - > int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, int count, void *param) > { > struct ioport *entry; > @@ -107,10 +82,6 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i > .node = RB_INT_INIT(port, port + count), > .ops = ops, > .priv = param, > - .dev_hdr = (struct device_header) { > - .bus_type = DEVICE_BUS_IOPORT, > - .data = generate_ioport_fdt_node, > - }, Since the dev_hdr field is not used anymore, maybe it could also be removed from struct ioport in include/kvm/ioport.h? Thanks, Alex > /* > * Start from 0 because ioport__unregister() doesn't decrement > * the reference count. > @@ -123,15 +94,10 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i > r = ioport_insert(&ioport_tree, entry); > if (r < 0) > goto out_free; > - r = device__register(&entry->dev_hdr); > - if (r < 0) > - goto out_remove; > mutex_unlock(&ioport_lock); > > return port; > > -out_remove: > - ioport_remove(&ioport_tree, entry); > out_free: > free(entry); > mutex_unlock(&ioport_lock);
On Thu, 11 Feb 2021 14:05:27 +0000 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Hi Andre, > > On 12/10/20 2:28 PM, Andre Przywara wrote: > > The ioport routines support a special way of registering FDT node > > generator functions. There is no reason to have this separate from the > > already existing way via the device header. > > > > Now that the only user of this special ioport variety has been > > transferred, we can retire this code, to simplify ioport handling. > > One comment below, but otherwise very nice cleanup. > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > include/kvm/ioport.h | 4 ---- > > ioport.c | 34 ---------------------------------- > > 2 files changed, 38 deletions(-) > > > > diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h > > index d0213541..a61038e2 100644 > > --- a/include/kvm/ioport.h > > +++ b/include/kvm/ioport.h > > @@ -29,10 +29,6 @@ struct ioport { > > struct ioport_operations { > > bool (*io_in)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); > > bool (*io_out)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); > > - void (*generate_fdt_node)(struct ioport *ioport, void *fdt, > > - void (*generate_irq_prop)(void *fdt, > > - u8 irq, > > - enum irq_type)); > > }; > > > > void ioport__map_irq(u8 *irq); > > diff --git a/ioport.c b/ioport.c > > index 667e8386..b98836d3 100644 > > --- a/ioport.c > > +++ b/ioport.c > > @@ -56,7 +56,6 @@ static struct ioport *ioport_get(struct rb_root *root, u64 addr) > > /* Called with ioport_lock held. */ > > static void ioport_unregister(struct rb_root *root, struct ioport *data) > > { > > - device__unregister(&data->dev_hdr); > > ioport_remove(root, data); > > free(data); > > } > > @@ -70,30 +69,6 @@ static void ioport_put(struct rb_root *root, struct ioport *data) > > mutex_unlock(&ioport_lock); > > } > > > > -#ifdef CONFIG_HAS_LIBFDT > > -static void generate_ioport_fdt_node(void *fdt, > > - struct device_header *dev_hdr, > > - void (*generate_irq_prop)(void *fdt, > > - u8 irq, > > - enum irq_type)) > > -{ > > - struct ioport *ioport = container_of(dev_hdr, struct ioport, dev_hdr); > > - struct ioport_operations *ops = ioport->ops; > > - > > - if (ops->generate_fdt_node) > > - ops->generate_fdt_node(ioport, fdt, generate_irq_prop); > > -} > > -#else > > -static void generate_ioport_fdt_node(void *fdt, > > - struct device_header *dev_hdr, > > - void (*generate_irq_prop)(void *fdt, > > - u8 irq, > > - enum irq_type)) > > -{ > > - die("Unable to generate device tree nodes without libfdt\n"); > > -} > > -#endif > > - > > int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, int count, void *param) > > { > > struct ioport *entry; > > @@ -107,10 +82,6 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i > > .node = RB_INT_INIT(port, port + count), > > .ops = ops, > > .priv = param, > > - .dev_hdr = (struct device_header) { > > - .bus_type = DEVICE_BUS_IOPORT, > > - .data = generate_ioport_fdt_node, > > - }, > > Since the dev_hdr field is not used anymore, maybe it could also be removed from > struct ioport in include/kvm/ioport.h? I could (seems to indeed still work without it), but this whole structure will go away with a later patch, so I didn't bother so far. That's why I am not sure it's useful to do this at this point then. Cheers, Andre > > /* > > * Start from 0 because ioport__unregister() doesn't decrement > > * the reference count. > > @@ -123,15 +94,10 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i > > r = ioport_insert(&ioport_tree, entry); > > if (r < 0) > > goto out_free; > > - r = device__register(&entry->dev_hdr); > > - if (r < 0) > > - goto out_remove; > > mutex_unlock(&ioport_lock); > > > > return port; > > > > -out_remove: > > - ioport_remove(&ioport_tree, entry); > > out_free: > > free(entry); > > mutex_unlock(&ioport_lock);
Hi Andre, On 2/17/21 3:54 PM, Andre Przywara wrote: > On Thu, 11 Feb 2021 14:05:27 +0000 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> Hi Andre, >> >> On 12/10/20 2:28 PM, Andre Przywara wrote: >>> The ioport routines support a special way of registering FDT node >>> generator functions. There is no reason to have this separate from the >>> already existing way via the device header. >>> >>> Now that the only user of this special ioport variety has been >>> transferred, we can retire this code, to simplify ioport handling. >> One comment below, but otherwise very nice cleanup. >> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> include/kvm/ioport.h | 4 ---- >>> ioport.c | 34 ---------------------------------- >>> 2 files changed, 38 deletions(-) >>> >>> diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h >>> index d0213541..a61038e2 100644 >>> --- a/include/kvm/ioport.h >>> +++ b/include/kvm/ioport.h >>> @@ -29,10 +29,6 @@ struct ioport { >>> struct ioport_operations { >>> bool (*io_in)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); >>> bool (*io_out)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); >>> - void (*generate_fdt_node)(struct ioport *ioport, void *fdt, >>> - void (*generate_irq_prop)(void *fdt, >>> - u8 irq, >>> - enum irq_type)); >>> }; >>> >>> void ioport__map_irq(u8 *irq); >>> diff --git a/ioport.c b/ioport.c >>> index 667e8386..b98836d3 100644 >>> --- a/ioport.c >>> +++ b/ioport.c >>> @@ -56,7 +56,6 @@ static struct ioport *ioport_get(struct rb_root *root, u64 addr) >>> /* Called with ioport_lock held. */ >>> static void ioport_unregister(struct rb_root *root, struct ioport *data) >>> { >>> - device__unregister(&data->dev_hdr); >>> ioport_remove(root, data); >>> free(data); >>> } >>> @@ -70,30 +69,6 @@ static void ioport_put(struct rb_root *root, struct ioport *data) >>> mutex_unlock(&ioport_lock); >>> } >>> >>> -#ifdef CONFIG_HAS_LIBFDT >>> -static void generate_ioport_fdt_node(void *fdt, >>> - struct device_header *dev_hdr, >>> - void (*generate_irq_prop)(void *fdt, >>> - u8 irq, >>> - enum irq_type)) >>> -{ >>> - struct ioport *ioport = container_of(dev_hdr, struct ioport, dev_hdr); >>> - struct ioport_operations *ops = ioport->ops; >>> - >>> - if (ops->generate_fdt_node) >>> - ops->generate_fdt_node(ioport, fdt, generate_irq_prop); >>> -} >>> -#else >>> -static void generate_ioport_fdt_node(void *fdt, >>> - struct device_header *dev_hdr, >>> - void (*generate_irq_prop)(void *fdt, >>> - u8 irq, >>> - enum irq_type)) >>> -{ >>> - die("Unable to generate device tree nodes without libfdt\n"); >>> -} >>> -#endif >>> - >>> int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, int count, void *param) >>> { >>> struct ioport *entry; >>> @@ -107,10 +82,6 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i >>> .node = RB_INT_INIT(port, port + count), >>> .ops = ops, >>> .priv = param, >>> - .dev_hdr = (struct device_header) { >>> - .bus_type = DEVICE_BUS_IOPORT, >>> - .data = generate_ioport_fdt_node, >>> - }, >> Since the dev_hdr field is not used anymore, maybe it could also be removed from >> struct ioport in include/kvm/ioport.h? > I could (seems to indeed still work without it), but this whole > structure will go away with a later patch, so I didn't bother so far. > That's why I am not sure it's useful to do this at this point then. Yes, you're totally right, no need to fiddle too much with it now because it will removed later: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex > > Cheers, > Andre > >>> /* >>> * Start from 0 because ioport__unregister() doesn't decrement >>> * the reference count. >>> @@ -123,15 +94,10 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i >>> r = ioport_insert(&ioport_tree, entry); >>> if (r < 0) >>> goto out_free; >>> - r = device__register(&entry->dev_hdr); >>> - if (r < 0) >>> - goto out_remove; >>> mutex_unlock(&ioport_lock); >>> >>> return port; >>> >>> -out_remove: >>> - ioport_remove(&ioport_tree, entry); >>> out_free: >>> free(entry); >>> mutex_unlock(&ioport_lock);
diff --git a/include/kvm/ioport.h b/include/kvm/ioport.h index d0213541..a61038e2 100644 --- a/include/kvm/ioport.h +++ b/include/kvm/ioport.h @@ -29,10 +29,6 @@ struct ioport { struct ioport_operations { bool (*io_in)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); bool (*io_out)(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size); - void (*generate_fdt_node)(struct ioport *ioport, void *fdt, - void (*generate_irq_prop)(void *fdt, - u8 irq, - enum irq_type)); }; void ioport__map_irq(u8 *irq); diff --git a/ioport.c b/ioport.c index 667e8386..b98836d3 100644 --- a/ioport.c +++ b/ioport.c @@ -56,7 +56,6 @@ static struct ioport *ioport_get(struct rb_root *root, u64 addr) /* Called with ioport_lock held. */ static void ioport_unregister(struct rb_root *root, struct ioport *data) { - device__unregister(&data->dev_hdr); ioport_remove(root, data); free(data); } @@ -70,30 +69,6 @@ static void ioport_put(struct rb_root *root, struct ioport *data) mutex_unlock(&ioport_lock); } -#ifdef CONFIG_HAS_LIBFDT -static void generate_ioport_fdt_node(void *fdt, - struct device_header *dev_hdr, - void (*generate_irq_prop)(void *fdt, - u8 irq, - enum irq_type)) -{ - struct ioport *ioport = container_of(dev_hdr, struct ioport, dev_hdr); - struct ioport_operations *ops = ioport->ops; - - if (ops->generate_fdt_node) - ops->generate_fdt_node(ioport, fdt, generate_irq_prop); -} -#else -static void generate_ioport_fdt_node(void *fdt, - struct device_header *dev_hdr, - void (*generate_irq_prop)(void *fdt, - u8 irq, - enum irq_type)) -{ - die("Unable to generate device tree nodes without libfdt\n"); -} -#endif - int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, int count, void *param) { struct ioport *entry; @@ -107,10 +82,6 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i .node = RB_INT_INIT(port, port + count), .ops = ops, .priv = param, - .dev_hdr = (struct device_header) { - .bus_type = DEVICE_BUS_IOPORT, - .data = generate_ioport_fdt_node, - }, /* * Start from 0 because ioport__unregister() doesn't decrement * the reference count. @@ -123,15 +94,10 @@ int ioport__register(struct kvm *kvm, u16 port, struct ioport_operations *ops, i r = ioport_insert(&ioport_tree, entry); if (r < 0) goto out_free; - r = device__register(&entry->dev_hdr); - if (r < 0) - goto out_remove; mutex_unlock(&ioport_lock); return port; -out_remove: - ioport_remove(&ioport_tree, entry); out_free: free(entry); mutex_unlock(&ioport_lock);
The ioport routines support a special way of registering FDT node generator functions. There is no reason to have this separate from the already existing way via the device header. Now that the only user of this special ioport variety has been transferred, we can retire this code, to simplify ioport handling. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- include/kvm/ioport.h | 4 ---- ioport.c | 34 ---------------------------------- 2 files changed, 38 deletions(-)