On 5/10/24 15:07, Greg KH wrote:
On Fri, May 10, 2024 at 02:54:49PM +0200, Przemek Kitszel wrote:
+static ssize_t auxiliary_irq_mode_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct auxiliary_irq_info *info =
+ container_of(attr, struct auxiliary_irq_info, sysfs_attr);
+
+ if (refcount_read(xa_load(&irqs, info->irq)) > 1)
refcount combined with xa? That feels wrong, why is refcount used for
this at all?
Not long ago I commented on similar usage for ice driver,
~"since you are locking anyway this could be a plain counter",
and author replied
~"additional semantics (like saturation) of refcount make me feel warm
and fuzzy" (sorry if misquoting too much).
That convinced me back then, so I kept quiet about that here.
But why is this being incremented / decremented at all? What is that
for?
[global]
This is just a counter, it is used to tell if given IRQ is shared or
exclusive. Hence there is a global xarray for that.
And my argument is for this case precisely.
[other]
There is also a separate xarray for each auxdev (IIRC) which is used as
generic dynamic container [that stores sysfs attrs], any other would
work (with different characteristics), but I see no problems with
picking xarray here.
The "use least powerful option" rule of thumb is perhaps more important.
Yes, but use a refcount properly if needed, I can't figure out why a
refcount is needed here at all, which is not a good sign.
+ refcount_set(new_ref, 1);
+ ref = __xa_cmpxchg(&irqs, irq, NULL, new_ref, GFP_KERNEL);
+ if (ref) {
+ kfree(new_ref);
+ if (xa_is_err(ref)) {
+ ret = xa_err(ref);
+ goto out;
+ }
+
+ /* Another thread beat us to creating the enrtry. */
+ refcount_inc(ref);
How can that happen? Why not just use a normal simple lock for all of
this so you don't have to mess with refcounts at all? This is not
performance-relevent code at all, but yet with a refcount you cause
almost the same issues that a normal lock would have, plus the increased
complexity of all of the surrounding code (like this, and the crazy
__xa_cmpxchg() call)
Make this simple please.
I find current API of xarray not ideal for this use case, and would like
to fix it, but let me write a proper RFC to don't derail (or slow down)
this series.
Why do you need to use an xarray here at all? Why isn't this just tied
directly to the aux device instead?
for [global] above I find xarray suitable soultion, for the [other] I'll
leave defending it to @Shay :)
thanks,
greg k-h
