| Message ID | 20240612085629.5015-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: Resource helper improvements | expand |
On Wed, 2024-06-12 at 11:56 +0300, Ilpo Järvinen wrote: > Setting the end address for a resource with a given size lacks a > helper > and is therefore open coded unlike the getter side which has a helper > for resource size calculation. "open coded"? How about "coded manually unlike [...]" > Also, almost all callsites that > calculate end address for a resource also set the start address right "an end address" or "the end address"? > before it like this: > > res->start = start_addr; > res->end = res->start + size - 1; > > Thus, add resource_set_range(res, start_addr, size) that sets the > start > address and calculates the end address to simplify this often > repeated > fragment. In addition, introduce resource_set_size() for the cases > where setting the start address of the resource is not necessary but > note resource_set_range() is preferred. "note"? I don't fully get that sentence. Looks like a cool little improvement otherwise :) P. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > include/linux/ioport.h | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index db7fe25f3370..2a1d33ad151c 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -216,6 +216,38 @@ struct resource *lookup_resource(struct resource > *root, resource_size_t start); > int adjust_resource(struct resource *res, resource_size_t start, > resource_size_t size); > resource_size_t resource_alignment(struct resource *res); > + > +/** > + * resource_set_size - Calculates resource end address from size and > start address > + * @res: The resource descriptor > + * @size: The size of the resource > + * > + * Calculates the end address for @res based on @size. > + * > + * Note: The start address of @res must be set when calling this > function. > + * Use resource_set_range() if setting both the start address and > @size. > + */ > +static inline void resource_set_size(struct resource *res, > resource_size_t size) > +{ > + res->end = res->start + size - 1; > +} > + > +/** > + * resource_set_range - Sets resource start and end addresses > + * @res: The resource descriptor > + * @start: The start address for the resource > + * @size: The size of the resource > + * > + * Sets @res start address and calculates the end address based on > @size. > + */ > +static inline void resource_set_range(struct resource *res, > + resource_size_t start, > + resource_size_t size) > +{ > + res->start = start; > + resource_set_size(res, size); > +} > + > static inline resource_size_t resource_size(const struct resource > *res) > { > return res->end - res->start + 1;
On Wed, 12 Jun 2024 11:56:27 +0300 Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > Setting the end address for a resource with a given size lacks a helper > and is therefore open coded unlike the getter side which has a helper > for resource size calculation. Also, almost all callsites that > calculate end address for a resource also set the start address right > before it like this: > > res->start = start_addr; > res->end = res->start + size - 1; > > Thus, add resource_set_range(res, start_addr, size) that sets the start > address and calculates the end address to simplify this often repeated > fragment. In addition, introduce resource_set_size() for the cases > where setting the start address of the resource is not necessary but > note resource_set_range() is preferred. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> We have a bunch of cases of this in CXL. Adding this helper seems like a good idea to me. I'm not sure the odd semantics of resource_set_size() are a good idea. Maybe it could by naming hint that it's relying internally on size already being set. resource_update_size() for instance might make people think or perhaps that's just more obscure. Meh, I've argued myself around to there not being a better name. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>` > --- > include/linux/ioport.h | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index db7fe25f3370..2a1d33ad151c 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -216,6 +216,38 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start); > int adjust_resource(struct resource *res, resource_size_t start, > resource_size_t size); > resource_size_t resource_alignment(struct resource *res); > + > +/** > + * resource_set_size - Calculates resource end address from size and start address > + * @res: The resource descriptor > + * @size: The size of the resource > + * > + * Calculates the end address for @res based on @size. > + * > + * Note: The start address of @res must be set when calling this function. > + * Use resource_set_range() if setting both the start address and @size. > + */ > +static inline void resource_set_size(struct resource *res, resource_size_t size) > +{ > + res->end = res->start + size - 1; > +} > + > +/** > + * resource_set_range - Sets resource start and end addresses > + * @res: The resource descriptor > + * @start: The start address for the resource > + * @size: The size of the resource > + * > + * Sets @res start address and calculates the end address based on @size. > + */ > +static inline void resource_set_range(struct resource *res, > + resource_size_t start, > + resource_size_t size) > +{ > + res->start = start; > + resource_set_size(res, size); > +} > + > static inline resource_size_t resource_size(const struct resource *res) > { > return res->end - res->start + 1;
On Thu, 13 Jun 2024, Jonathan Cameron wrote: > On Wed, 12 Jun 2024 11:56:27 +0300 > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > Setting the end address for a resource with a given size lacks a helper > > and is therefore open coded unlike the getter side which has a helper > > for resource size calculation. Also, almost all callsites that > > calculate end address for a resource also set the start address right > > before it like this: > > > > res->start = start_addr; > > res->end = res->start + size - 1; > > > > Thus, add resource_set_range(res, start_addr, size) that sets the start > > address and calculates the end address to simplify this often repeated > > fragment. In addition, introduce resource_set_size() for the cases > > where setting the start address of the resource is not necessary but > > note resource_set_range() is preferred. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > We have a bunch of cases of this in CXL. Adding this helper seems like > a good idea to me. Sadly this won't help struct range cases which feature the same math. > I'm not sure the odd semantics of resource_set_size() are a good idea. > Maybe it could by naming hint that it's relying internally on > size already being set. > > resource_update_size() for instance might make people think or perhaps > that's just more obscure. Meh, I've argued myself around to there > not being a better name. Yeah, I tried to figure out solution to this very challenge too, but alas, couldn't really find any good solution to it. __ prefix would have kind of conveyed the meaning that you better know what you're doing but as some people oppose __ too, I didn't want to stir that pot. So it is what it is. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>` FYI, I dropped the extra ` from that (no need to reply because of it). Thanks for the reviews.
diff --git a/include/linux/ioport.h b/include/linux/ioport.h index db7fe25f3370..2a1d33ad151c 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -216,6 +216,38 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start); int adjust_resource(struct resource *res, resource_size_t start, resource_size_t size); resource_size_t resource_alignment(struct resource *res); + +/** + * resource_set_size - Calculates resource end address from size and start address + * @res: The resource descriptor + * @size: The size of the resource + * + * Calculates the end address for @res based on @size. + * + * Note: The start address of @res must be set when calling this function. + * Use resource_set_range() if setting both the start address and @size. + */ +static inline void resource_set_size(struct resource *res, resource_size_t size) +{ + res->end = res->start + size - 1; +} + +/** + * resource_set_range - Sets resource start and end addresses + * @res: The resource descriptor + * @start: The start address for the resource + * @size: The size of the resource + * + * Sets @res start address and calculates the end address based on @size. + */ +static inline void resource_set_range(struct resource *res, + resource_size_t start, + resource_size_t size) +{ + res->start = start; + resource_set_size(res, size); +} + static inline resource_size_t resource_size(const struct resource *res) { return res->end - res->start + 1;
Setting the end address for a resource with a given size lacks a helper and is therefore open coded unlike the getter side which has a helper for resource size calculation. Also, almost all callsites that calculate end address for a resource also set the start address right before it like this: res->start = start_addr; res->end = res->start + size - 1; Thus, add resource_set_range(res, start_addr, size) that sets the start address and calculates the end address to simplify this often repeated fragment. In addition, introduce resource_set_size() for the cases where setting the start address of the resource is not necessary but note resource_set_range() is preferred. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- include/linux/ioport.h | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)