Re: [PATCH 00/20] x86: address -Wmissing-prototype warnings
On Fri, May 19, 2023 at 12:56 AM Dave Hansen wrote: > On 5/16/23 12:35, Arnd Bergmann wrote: > I picked up the ones that were blatantly obvious, but left out 03, 04, > 10, 12 and 19 for the moment. Btw, there is series that went unnoticed https://lore.kernel.org/all/2029110017.48510-1-andriy.shevche...@linux.intel.com/ I dunno why. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API
On Fri, Sep 16, 2022 at 10:09 AM Liang He wrote:
> At 2022-09-16 13:38:39, "Andy Shevchenko" wrote:
> >On Fri, Sep 16, 2022 at 5:02 AM Liang He wrote:
> >> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" wrote:
> >> >On 9/14/22 7:23 PM, Liang He wrote:
...
> >> >> static inline bool jailhouse_paravirt(void)
> >> >> {
> >> >> -return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> >> +struct device_node *np = of_find_compatible_node(NULL, NULL,
> >> >> "jailhouse,cell");
> >> >> +
> >> >> +of_node_put(np);
> >> >> +
> >> >> +return np;
> >> >> }
> >> >
> >> >Thank you for the fix, but returning a pointer from a function with a
> >> >bool return type looks odd. Can we also fix that up please?
> >>
> >> Thanks for your review, how about following patch:
> >>
> >> - return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> + struct device_node *np = of_find_compatible_node(NULL, NULL,
> >> "jailhouse,cell");
> >> +
> >> + of_node_put(np);
> >> +
> >> + return (np==NULL);
>
> >This will be opposite to the above. Perhaps you wanted
>
> Sorry, I wanted to use 'np!=NULL'
>
> > return !!np;
> >
> >Also possible (but why?)
> >
> > return np ? true : false;
>
> So, can I chose 'return np?true: false;' as the final patch?
Of course you can, it's up to the maintainer(s) what to accept.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Re: [PATCH] jailhouse: Hold reference returned from of_find_xxx API
On Fri, Sep 16, 2022 at 5:02 AM Liang He wrote:
> At 2022-09-16 07:29:06, "Srivatsa S. Bhat" wrote:
> >On 9/14/22 7:23 PM, Liang He wrote:
..
> >> static inline bool jailhouse_paravirt(void)
> >> {
> >> -return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> >> +struct device_node *np = of_find_compatible_node(NULL, NULL,
> >> "jailhouse,cell");
> >> +
> >> +of_node_put(np);
> >> +
> >> +return np;
> >> }
> >
> >Thank you for the fix, but returning a pointer from a function with a
> >bool return type looks odd. Can we also fix that up please?
> >
>
> Thanks for your review, how about following patch:
>
> - return of_find_compatible_node(NULL, NULL, "jailhouse,cell");
> + struct device_node *np = of_find_compatible_node(NULL, NULL,
> "jailhouse,cell");
> +
> + of_node_put(np);
> +
> + return (np==NULL);
This will be opposite to the above. Perhaps you wanted
return !!np;
Also possible (but why?)
return np ? true : false;
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 1/1] iommu/virtio: Do not dereference fwnode in struct device
On Tue, Aug 09, 2022 at 08:20:48AM +0100, Jean-Philippe Brucker wrote: > On Mon, Aug 01, 2022 at 07:51:42PM +0300, Andy Shevchenko wrote: > > In order to make the underneath API easier to change in the future, > > prevent users from dereferencing fwnode from struct device. > > Instead, use the specific device_match_fwnode() API for that. > Reviewed-by: Jean-Philippe Brucker Thanks! Can it be applied now? -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v1 1/1] iommu/virtio: Do not dereference fwnode in struct device
In order to make the underneath API easier to change in the future,
prevent users from dereferencing fwnode from struct device.
Instead, use the specific device_match_fwnode() API for that.
Signed-off-by: Andy Shevchenko
---
drivers/iommu/virtio-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 08eeafc9529f..9fe723f55213 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -925,7 +925,7 @@ static struct virtio_driver virtio_iommu_drv;
static int viommu_match_node(struct device *dev, const void *data)
{
- return dev->parent->fwnode == data;
+ return device_match_fwnode(dev->parent, data);
}
static struct viommu_dev *viommu_get_by_fwnode(struct fwnode_handle *fwnode)
--
2.35.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 01/12] driver: platform: Add helper for safer setting of driver_override
On Mon, Apr 4, 2022 at 12:34 PM Krzysztof Kozlowski
wrote:
> On 04/04/2022 11:16, Andy Shevchenko wrote:
> > On Sun, Apr 3, 2022 at 9:38 PM Krzysztof Kozlowski
> > wrote:
...
> >> +int driver_set_override(struct device *dev, const char **override,
> >> + const char *s, size_t len)
> >> +{
> >> + const char *new, *old;
> >> + char *cp;
> >
> >> + if (!override || !s)
> >> + return -EINVAL;
> >
> > Still not sure if we should distinguish (s == NULL && len == 0) from
> > (s != NULL && len == 0).
> > Supplying the latter seems confusing (yes, I see that in the old code).
> > Perhaps
> > !s test, in case you want to leave it, should be also commented.
>
> The old semantics were focused on sysfs usage, so clearing is by passing
> an empty string. In the case of sysfs empty string is actually "\n". I
> intend to keep the semantics also for the in-kernel usage and in such
> case empty string can be also "".
>
> If I understand your comment correctly, you propose to change it to NULL
> for in-kernel usage, but that would change the semantics.
Yes. It's also possible to have a wrapper for sysfs use.
> > Another approach is to split above to two checks and move !s after !len.
>
> I don't follow why... The !override and !s are invalid uses. !len is a
> valid user for internal callers, just like "\n" is for sysfs.
I understand but always supplying s maybe an overhead for in-kernel usages.
In any case, it's not critical right now, just a remark that it can be modified.
> >> + /*
> >> +* The stored value will be used in sysfs show callback
> >> (sysfs_emit()),
> >> +* which has a length limit of PAGE_SIZE and adds a trailing
> >> newline.
> >> +* Thus we can store one character less to avoid truncation during
> >> sysfs
> >> +* show.
> >> +*/
> >> + if (len >= (PAGE_SIZE - 1))
> >> + return -EINVAL;
> >
> > Perhaps explain the case in the comment here?
>
> You mean the case we discuss here (to clear override with "")? Sure.
Yep. Before the below check.
> >> + if (!len) {
> >> + device_lock(dev);
> >> + old = *override;
> >> + *override = NULL;
> >
> >> + device_unlock(dev);
> >> + goto out_free;
> >
> > You may deduplicate this one, by
> >
> >goto out_unlock_free;
> >
> > But I understand your intention to keep lock-unlock in one place, so
> > perhaps dropping that label would be even better in this case and
> > keeping it
>
> Yes, exactly.
>
> >kfree(old);
> >return 0;
> >
> > here instead of goto.
>
> Slightly more code, but indeed maybe easier to follow. I'll do like this.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 01/12] driver: platform: Add helper for safer setting of driver_override
On Sun, Apr 3, 2022 at 9:38 PM Krzysztof Kozlowski
wrote:
>
> Several core drivers and buses expect that driver_override is a
> dynamically allocated memory thus later they can kfree() it.
>
> However such assumption is not documented, there were in the past and
> there are already users setting it to a string literal. This leads to
> kfree() of static memory during device release (e.g. in error paths or
> during unbind):
>
> kernel BUG at ../mm/slub.c:3960!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> ...
> (kfree) from [] (platform_device_release+0x88/0xb4)
> (platform_device_release) from [] (device_release+0x2c/0x90)
> (device_release) from [] (kobject_put+0xec/0x20c)
> (kobject_put) from [] (exynos5_clk_probe+0x154/0x18c)
> (exynos5_clk_probe) from [] (platform_drv_probe+0x6c/0xa4)
> (platform_drv_probe) from [] (really_probe+0x280/0x414)
> (really_probe) from [] (driver_probe_device+0x78/0x1c4)
> (driver_probe_device) from [] (bus_for_each_drv+0x74/0xb8)
> (bus_for_each_drv) from [] (__device_attach+0xd4/0x16c)
> (__device_attach) from [] (bus_probe_device+0x88/0x90)
> (bus_probe_device) from [] (device_add+0x3dc/0x62c)
> (device_add) from [] (of_platform_device_create_pdata+0x94/0xbc)
> (of_platform_device_create_pdata) from []
> (of_platform_bus_create+0x1a8/0x4fc)
> (of_platform_bus_create) from []
> (of_platform_bus_create+0x20c/0x4fc)
> (of_platform_bus_create) from []
> (of_platform_populate+0x84/0x118)
> (of_platform_populate) from []
> (of_platform_default_populate_init+0xa0/0xb8)
> (of_platform_default_populate_init) from []
> (do_one_initcall+0x8c/0x404)
>
> Provide a helper which clearly documents the usage of driver_override.
> This will allow later to reuse the helper and reduce the amount of
> duplicated code.
>
> Convert the platform driver to use a new helper and make the
> driver_override field const char (it is not modified by the core).
...
> +int driver_set_override(struct device *dev, const char **override,
> + const char *s, size_t len)
> +{
> + const char *new, *old;
> + char *cp;
> + if (!override || !s)
> + return -EINVAL;
Still not sure if we should distinguish (s == NULL && len == 0) from
(s != NULL && len == 0).
Supplying the latter seems confusing (yes, I see that in the old code). Perhaps
!s test, in case you want to leave it, should be also commented.
Another approach is to split above to two checks and move !s after !len.
> + /*
> +* The stored value will be used in sysfs show callback
> (sysfs_emit()),
> +* which has a length limit of PAGE_SIZE and adds a trailing newline.
> +* Thus we can store one character less to avoid truncation during
> sysfs
> +* show.
> +*/
> + if (len >= (PAGE_SIZE - 1))
> + return -EINVAL;
Perhaps explain the case in the comment here?
> + if (!len) {
> + device_lock(dev);
> + old = *override;
> + *override = NULL;
> + device_unlock(dev);
> + goto out_free;
You may deduplicate this one, by
goto out_unlock_free;
But I understand your intention to keep lock-unlock in one place, so
perhaps dropping that label would be even better in this case and
keeping it
kfree(old);
return 0;
here instead of goto.
> + }
> +
> + cp = strnchr(s, len, '\n');
> + if (cp)
> + len = cp - s;
> +
> + new = kstrndup(s, len, GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> +
> + device_lock(dev);
> + old = *override;
> + if (cp != s) {
> + *override = new;
> + } else {
> + kfree(new);
> + *override = NULL;
> + }
> + device_unlock(dev);
> +
> +out_free:
> + kfree(old);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5 01/11] driver: platform: Add helper for safer setting of driver_override
On Wed, Mar 16, 2022 at 5:06 PM Krzysztof Kozlowski
wrote:
...
> +int driver_set_override(struct device *dev, const char **override,
> + const char *s, size_t len)
> +{
> + const char *new, *old;
> + char *cp;
> + if (!dev || !override || !s)
> + return -EINVAL;
Sorry, I didn't pay much attention on this. First of all, I would drop
dev checks and simply require that dev should be valid. Do you expect
this can be called when dev is invalid? I would like to hear if it's
anything but theoretical. Second one, is the !s requirement. Do I
understand correctly that the string must be always present? But then
how we NULify the override? Is it possible? Third one is absence of
len check. See below.
> + /*
> +* The stored value will be used in sysfs show callback
> (sysfs_emit()),
> +* which has a length limit of PAGE_SIZE and adds a trailing newline.
> +* Thus we can store one character less to avoid truncation during
> sysfs
> +* show.
> +*/
> + if (len >= (PAGE_SIZE - 1))
> + return -EINVAL;
I would relax this to make sure we can use it if \n is within this limit.
> + cp = strnchr(s, len, '\n');
> + if (cp)
> + len = cp - s;
> +
> + new = kstrndup(s, len, GFP_KERNEL);
Here is a word about the len check.
> + if (!new)
If len == 0, this won't trigger and you have something very
interesting as a result.
One way is to use ZERO_PTR_OR_NULL() another is explicitly check for 0
and issue a (different?) error code.
> + return -ENOMEM;
> +
> + device_lock(dev);
> + old = *override;
> + if (cp != s) {
> + *override = new;
> + } else {
> + kfree(new);
> + *override = NULL;
> + }
> + device_unlock(dev);
> +
> + kfree(old);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 05/11] PCI: Use driver_set_override() instead of open-coding
On Sat, Mar 12, 2022 at 4:09 PM Krzysztof Kozlowski
wrote:
>
> Use a helper to set driver_override to reduce amount of duplicated code.
the amount
> Make the driver_override field const char, because it is not modified by
> the core and it matches other subsystems.
Seems like mine #4 here
https://gist.github.com/andy-shev/a2cb1ee4767d6d2f5d20db53ecb9aabc :-)
Reviewed-by: Andy Shevchenko
Thanks!
> Signed-off-by: Krzysztof Kozlowski
> Acked-by: Bjorn Helgaas
> ---
> drivers/pci/pci-sysfs.c | 28
> include/linux/pci.h | 6 +-
> 2 files changed, 9 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 602f0fb0b007..5c42965c32c2 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -567,31 +567,11 @@ static ssize_t driver_override_store(struct device *dev,
> const char *buf, size_t count)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> - char *driver_override, *old, *cp;
> -
> - /* We need to keep extra room for a newline */
> - if (count >= (PAGE_SIZE - 1))
> - return -EINVAL;
> -
> - driver_override = kstrndup(buf, count, GFP_KERNEL);
> - if (!driver_override)
> - return -ENOMEM;
> -
> - cp = strchr(driver_override, '\n');
> - if (cp)
> - *cp = '\0';
> -
> - device_lock(dev);
> - old = pdev->driver_override;
> - if (strlen(driver_override)) {
> - pdev->driver_override = driver_override;
> - } else {
> - kfree(driver_override);
> - pdev->driver_override = NULL;
> - }
> - device_unlock(dev);
> + int ret;
>
> - kfree(old);
> + ret = driver_set_override(dev, >driver_override, buf, count);
> + if (ret)
> + return ret;
>
> return count;
> }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60d423d8f0c4..415491fb85f4 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -516,7 +516,11 @@ struct pci_dev {
> u16 acs_cap;/* ACS Capability offset */
> phys_addr_t rom;/* Physical address if not from BAR */
> size_t romlen; /* Length if not from BAR */
> - char*driver_override; /* Driver name to force a match */
> + /*
> +* Driver name to force a match. Do not set directly, because core
> +* frees it. Use driver_set_override() to set or clear it.
> +*/
> + const char *driver_override;
>
> unsigned long priv_flags; /* Private flags for the PCI driver */
>
> --
> 2.32.0
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 01/11] driver: platform: Add helper for safer setting of driver_override
On Sat, Mar 12, 2022 at 5:16 PM Krzysztof Kozlowski
wrote:
>
> Several core drivers and buses expect that driver_override is a
> dynamically allocated memory thus later they can kfree() it.
>
> However such assumption is not documented, there were in the past and
> there are already users setting it to a string literal. This leads to
> kfree() of static memory during device release (e.g. in error paths or
> during unbind):
>
> kernel BUG at ../mm/slub.c:3960!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> ...
> (kfree) from [] (platform_device_release+0x88/0xb4)
> (platform_device_release) from [] (device_release+0x2c/0x90)
> (device_release) from [] (kobject_put+0xec/0x20c)
> (kobject_put) from [] (exynos5_clk_probe+0x154/0x18c)
> (exynos5_clk_probe) from [] (platform_drv_probe+0x6c/0xa4)
> (platform_drv_probe) from [] (really_probe+0x280/0x414)
> (really_probe) from [] (driver_probe_device+0x78/0x1c4)
> (driver_probe_device) from [] (bus_for_each_drv+0x74/0xb8)
> (bus_for_each_drv) from [] (__device_attach+0xd4/0x16c)
> (__device_attach) from [] (bus_probe_device+0x88/0x90)
> (bus_probe_device) from [] (device_add+0x3dc/0x62c)
> (device_add) from [] (of_platform_device_create_pdata+0x94/0xbc)
> (of_platform_device_create_pdata) from []
> (of_platform_bus_create+0x1a8/0x4fc)
> (of_platform_bus_create) from []
> (of_platform_bus_create+0x20c/0x4fc)
> (of_platform_bus_create) from []
> (of_platform_populate+0x84/0x118)
> (of_platform_populate) from []
> (of_platform_default_populate_init+0xa0/0xb8)
> (of_platform_default_populate_init) from []
> (do_one_initcall+0x8c/0x404)
> (do_one_initcall) from [] (kernel_init_freeable+0x3d0/0x4d8)
> (kernel_init_freeable) from [] (kernel_init+0x8/0x114)
> (kernel_init) from [] (ret_from_fork+0x14/0x20)
I believe you may remove these three.
> Provide a helper which clearly documents the usage of driver_override.
> This will allow later to reuse the helper and reduce amount of
the amount
> duplicated code.
> Convert the platform driver to use new helper and make the
a new
> driver_override field const char (it is not modified by the core).
...
> +/**
> + * driver_set_override() - Helper to set or clear driver override.
> + * @dev: Device to change
> + * @override: Address of string to change (e.g. >driver_override);
> + *The contents will be freed and hold newly allocated override.
> + * @s: NUL terminated string, new driver name to force a match, pass empty
NUL-terminated? (44 vs 115 occurrences)
> + * string to clear it
> + * @len: length of @s
> + *
> + * Helper to set or clear driver override in a device, intended for the cases
> + * when the driver_override field is allocated by driver/bus code.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int driver_set_override(struct device *dev, const char **override,
> + const char *s, size_t len)
> +{
> + const char *new, *old;
> + char *cp;
> +
> + if (!dev || !override || !s)
> + return -EINVAL;
> +
> + /*
> +* The stored value will be used in sysfs show callback
> (sysfs_emit()),
> +* which has a length limit of PAGE_SIZE and adds a trailing newline.
> +* Thus we can store one character less to avoid truncation during
> sysfs
> +* show.
> +*/
> + if (len >= (PAGE_SIZE - 1))
> + return -EINVAL;
> +
> + new = kstrndup(s, len, GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> +
> + cp = strchr(new, '\n');
> + if (cp)
> + *cp = '\0';
AFAIU you may reduce memory footprint by
cp = strnchr(new, len, '\n');
if (cp)
len = s - cp;
new = kstrndup(...);
> + device_lock(dev);
> + old = *override;
> + if (cp != new) {
> + *override = new;
> + } else {
> + kfree(new);
> + *override = NULL;
> + }
> + device_unlock(dev);
> +
> + kfree(old);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH bpf-next v2] net: don't include filter.h from net/sock.h
On Wed, Dec 29, 2021 at 09:20:12AM -0800, Jakub Kicinski wrote: > On Tue, 28 Dec 2021 17:33:39 -0800 Florian Fainelli wrote: > > It would be nice if we used the number of files rebuilt because of a > > header file change as another metric that the kernel is evaluated with > > from release to release (or even on a commit by commit basis). Food for > > thought. > > Maybe Andy has some thoughts, he has been working on dropping > unnecessary includes of kernel.h, it seems. With this [1] announcement I believe Ingo is the best to tell you if this is a right direction. > It'd be cool to plug something that'd warn us about significant > increases in dependencies into the patchwork build bot. > > I have one more small series which un-includes uapi/bpf.h from > netdevice.h at which point I hope we'll be largely in the clear > from build bot performance perspective. [1]: https://lore.kernel.org/lkml/ydifz+lmewets...@gmail.com/T/#u -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/4] soundwire: intel: Use auxiliary_device driver data helpers
On Tue, Dec 21, 2021 at 03:58:50PM -0800, David E. Box wrote:
> Use auxiliary_get_drvdata and auxiliary_set_drvdata helpers.
Reviewed-by: Andy Shevchenko
> Signed-off-by: David E. Box
> ---
> drivers/soundwire/intel.c | 8
> drivers/soundwire/intel_init.c | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 78037ffdb09b..d082d18e41a9 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1293,7 +1293,7 @@ static int intel_link_probe(struct auxiliary_device
> *auxdev,
> bus->ops = _intel_ops;
>
> /* set driver data, accessed by snd_soc_dai_get_drvdata() */
> - dev_set_drvdata(dev, cdns);
> + auxiliary_set_drvdata(auxdev, cdns);
>
> /* use generic bandwidth allocation algorithm */
> sdw->cdns.bus.compute_params = sdw_compute_params;
> @@ -1321,7 +1321,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
> {
> struct sdw_cdns_stream_config config;
> struct device *dev = >dev;
> - struct sdw_cdns *cdns = dev_get_drvdata(dev);
> + struct sdw_cdns *cdns = auxiliary_get_drvdata(auxdev);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_bus *bus = >bus;
> int link_flags;
> @@ -1463,7 +1463,7 @@ int intel_link_startup(struct auxiliary_device *auxdev)
> static void intel_link_remove(struct auxiliary_device *auxdev)
> {
> struct device *dev = >dev;
> - struct sdw_cdns *cdns = dev_get_drvdata(dev);
> + struct sdw_cdns *cdns = auxiliary_get_drvdata(auxdev);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_bus *bus = >bus;
>
> @@ -1488,7 +1488,7 @@ int intel_link_process_wakeen_event(struct
> auxiliary_device *auxdev)
> void __iomem *shim;
> u16 wake_sts;
>
> - sdw = dev_get_drvdata(dev);
> + sdw = auxiliary_get_drvdata(auxdev);
> bus = >cdns.bus;
>
> if (bus->prop.hw_disabled || !sdw->startup_done) {
> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index e329022e1669..d99807765dfe 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -244,7 +244,7 @@ static struct sdw_intel_ctx
> goto err;
>
> link = >link_res;
> - link->cdns = dev_get_drvdata(>auxdev.dev);
> + link->cdns = auxiliary_get_drvdata(>auxdev);
>
> if (!link->cdns) {
> dev_err(>dev, "failed to get link->cdns\n");
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/17] Add memberof(), split some headers, and slightly simplify code
On Fri, Nov 19, 2021 at 05:12:19PM +0100, Alejandro Colomar (man-pages) wrote: > On 11/19/21 16:57, Arnd Bergmann wrote: ... > > On the plus side, I did see something on the order of a 30% > > compile speed improvement with clang, which is insane > > given that this only removed dead definitions. > > Huh! > > I'd like to see the kernel some day > not having _any_ hidden dependencies. It's neither feasible nor practical. If we know the hard dependencies between headers, why should we not use implicit inclusion? We all know that bitmap.h includes bitops.h and this is good and a must, why to avoid this? -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/17] Add memberof(), split some headers, and slightly simplify code
On Fri, Nov 19, 2021 at 05:22:48PM +0100, Alejandro Colomar (man-pages) wrote: > > > On 11/19/21 17:18, Arnd Bergmann wrote: > > On Fri, Nov 19, 2021 at 5:10 PM Andy Shevchenko > > wrote: > >> On Fri, Nov 19, 2021 at 04:57:46PM +0100, Arnd Bergmann wrote: > > > >>> The main problem with this approach is that as soon as you start > >>> actually reducing the unneeded indirect includes, you end up with > >>> countless .c files that no longer build because they are missing a > >>> direct include for something that was always included somewhere > >>> deep underneath, so I needed a second set of scripts to add > >>> direct includes to every .c file. > >> > >> Can't it be done with cocci support? > > > > There are many ways of doing it, but they all tend to suffer from the > > problem of identifying which headers are actually needed based on > > the contents of a file, and also figuring out where to put the extra > > #include if there are complex #ifdefs. > > > > For reference, see below for the naive pattern matching I tried. > > This is obviously incomplete and partially wrong. > > FYI, if you may not know the tool, > theres include-what-you-use(1) (a.k.a. iwyu(1))[1], > although it is still not mature, > and I'm helping improve it a bit. Yes, I know the tool, but it produces insanity. Jonathan (maintainer of IIO subsystem) actually found it useful after manual work applied. Perhaps you can chat with him about usage of it in the Linux kernel. > If I understood better the kernel Makefiles, > I'd try it. > > You can try it yourselves. > I still can't use it for my own code, > since it has a lot of false positives. > [1]: <https://include-what-you-use.org/> -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/17] Add memberof(), split some headers, and slightly simplify code
On Fri, Nov 19, 2021 at 04:57:46PM +0100, Arnd Bergmann wrote: > On Fri, Nov 19, 2021 at 4:06 PM Alejandro Colomar (man-pages) > wrote: > > On 11/19/21 15:47, Arnd Bergmann wrote: > > > On Fri, Nov 19, 2021 at 12:36 PM Alejandro Colomar > > > > Yes, I would like to untangle the dependencies. > > > > The main reason I started doing this splitting > > is because I wouldn't be able to include > > in some headers, > > because it pulled too much stuff that broke unrelated things. > > > > So that's why I started from there. > > > > I for example would like to get NULL in memberof() > > without puling anything else, > > so makes sense for that. > > > > It's clear that every .c wants NULL, > > but it's not so clear that every .c wants > > everything that pulls indirectly. > > From what I can tell, linux/stddef.h is tiny, I don't think it's really > worth optimizing this part. I have spent some time last year > trying to untangle some of the more interesting headers, but ended > up not completing this as there are some really hard problems > once you start getting to the interesting bits. > > The approach I tried was roughly: > > - For each header in the kernel, create a preprocessed version > that includes all the indirect includes, from that start a set > of lookup tables that record which header is eventually included > by which ones, and the size of each preprocessed header in > bytes > > - For a given kernel configuration (e.g. defconfig or allmodconfig) > that I'm most interested in, look at which files are built, and what > the direct includes are in the source files. > > - Sort the headers by the product of the number of direct includes > and the preprocessed size: the largest ones are those that are > worth looking at first. > > - use graphviz to visualize the directed graph showing the includes > between the top 100 headers in that list. You get something like > I had in [1], or the version afterwards at [2]. > > - split out unneeded indirect includes from the headers in the center > of that graph, typically by splitting out struct definitions. > > - repeat. > > The main problem with this approach is that as soon as you start > actually reducing the unneeded indirect includes, you end up with > countless .c files that no longer build because they are missing a > direct include for something that was always included somewhere > deep underneath, so I needed a second set of scripts to add > direct includes to every .c file. Can't it be done with cocci support? > On the plus side, I did see something on the order of a 30% > compile speed improvement with clang, which is insane > given that this only removed dead definitions. Thumb up! > > But I'll note that linux/fs.h, linux/sched.h, linux/mm.h are > > interesting headers for further splitting. > > > > > > BTW, I also have a longstanding doubt about > > how header files are organized in the kernel, > > and which headers can and cannot be included > > from which other files. > > > > For example I see that files in samples or scripts or tools, > > that redefine many things such as offsetof() or ARRAY_SIZE(), > > and I don't know if there's a good reason for that, > > or if I should simply remove all that stuff and > > include everywhere I see offsetof() being used. > > The main issue here is that user space code should not > include anything outside of include/uapi/ and arch/*/include/uapi/ > > offsetof() is defined in include/linux/stddef.h, so this is by > definition not accessible here. It appears that there is also > an include/uapi/linux/stddef.h that is really strange because > it includes linux/compiler_types.h, which in turn is outside > of uapi/. This should probably be fixed. > > Arnd > > [1] > https://drive.google.com/file/d/14IKifYDadg2W5fMsefxr4373jizo9bLl/view?usp=sharing > [2] > https://drive.google.com/file/d/1pWQcv3_ZXGqZB8ogV-JOfoV-WJN2UNnd/view?usp=sharing -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/17] Add memberof(), split some headers, and slightly simplify code
On Fri, Nov 19, 2021 at 04:06:27PM +0100, Alejandro Colomar (man-pages) wrote: > Hi Arnd, > > On 11/19/21 15:47, Arnd Bergmann wrote: > > On Fri, Nov 19, 2021 at 12:36 PM Alejandro Colomar > > wrote: > >> > >> Alejandro Colomar (17): > >> linux/container_of.h: Add memberof(T, m) > >> Use memberof(T, m) instead of explicit NULL dereference > >> Replace some uses of memberof() by its wrappers > >> linux/memberof.h: Move memberof() to separate header > >> linux/typeof_member.h: Move typeof_member() to a separate header > >> Simplify sizeof(typeof_member()) to sizeof_field() > >> linux/NULL.h: Move NULL to a separate header > >> linux/offsetof.h: Move offsetof(T, m) to a separate header > >> linux/offsetof.h: Implement offsetof() in terms of memberof() > >> linux/container_of.h: Implement container_of_safe() in terms of > >> container_of() > >> linux/container_of.h: Cosmetic > >> linux/container_of.h: Remove unnecessary cast to (void *) > > > > My feeling is that this takes the separation too far: by having this many > > header > > files that end up being included from practically every single .c file > > in the kernel, > > I think you end up making compile speed worse overall. > > > > If your goal is to avoid having to recompile as much of the kernel > > after touching > > a header, I think a better approach is to help untangle the dependencies, > > e.g. > > by splitting out type definitions from headers with inline functions (most > > indirect header dependencies are on type definitions) and by focusing on > > linux/fs.h, linux/sched.h, linux/mm.h and how they interact with the rest > > of the > > headers. At the moment, these are included in most .c files and they in turn > > include a ton of other headers. > > Yes, I would like to untangle the dependencies. > > The main reason I started doing this splitting > is because I wouldn't be able to include > in some headers, > because it pulled too much stuff that broke unrelated things. > > So that's why I started from there. > > I for example would like to get NULL in memberof() > without puling anything else, > so makes sense for that. I don't believe that the code that uses NULL won't include types.h. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 00/17] Add memberof(), split some headers, and slightly simplify code
On Fri, Nov 19, 2021 at 02:16:03PM +0100, Alejandro Colomar (man-pages) wrote: > On 11/19/21 13:47, Jani Nikula wrote: > > On Fri, 19 Nov 2021, Alejandro Colomar wrote: ... > Patch set 1: > - Add with memberof() > - Split offsetof() to > - Split offsetofend() to > - Split typeof_member() to > - Split sizeof_field() to > - Split NULL to > - Split ARRAY_SIZE() to Isn't it way too small granularity? I agree on having the separate header for ARRAY_SIZE() and it was discussed, but the rest... -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V6] gpio: virtio: Add IRQ support
On Thu, Oct 21, 2021 at 12:52 PM Viresh Kumar wrote:
> On 21-10-21, 12:42, Andy Shevchenko wrote:
> > On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar
> > wrote:
> > > On 20-10-21, 18:10, Andy Shevchenko wrote:
...
> > > > > struct virtio_gpio_config {
> > > > > __le16 ngpio;
> > > > > __u8 padding[2];
> > > > > @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names {
> > > > > __u8 value[];
> > > > > };
> > > > >
> > > > > +/* Virtio GPIO IRQ Request / Response */
> > > > > +struct virtio_gpio_irq_request {
> > > > > + __le16 gpio;
> > > > > +};
> > > > > +
> > > > > +struct virtio_gpio_irq_response {
> > > > > + __u8 status;
> > > > > +};
> > > > >
> > > > I’m wondering if those above should be packed.
> > >
> > > You are talking about the newly added ones or the ones before ?
> > >
> > > In any case, they are all already packed (i.e. they have explicit
> > > padding wherever required) and properly aligned. Compiler won't add
> > > any other padding to them.
> >
> > Is it only for 64-bit to 64-bit communications?
>
> That's what I have been looking at.
>
> > If there is a possibility to have 32-bit to 64-bit or vice versa
> > communication you have a problem.
>
> This should work as well.
>
> The structure will get aligned to the size of largest element and each
> element will be aligned to itself. I don't see how this will break
> even in case of 32/64 bit communication.
I admit I haven't looked into the specification, but in the past we
had had quite an issue exactly in GPIO on kernel side because of this
kind of design mistake. The problem here if in the future one wants to
supply more than one item at a time, it will be not possible with this
interface. Yes, I understand that in current design it's rather missed
scalability, but hey, I believe in the future we may need
performance-wise calls.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V6] gpio: virtio: Add IRQ support
On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar wrote:
> On 20-10-21, 18:10, Andy Shevchenko wrote:
> > On Wednesday, October 20, 2021, Viresh Kumar
> > wrote:
...
> > > + case IRQ_TYPE_NONE:
> > > + type = VIRTIO_GPIO_IRQ_TYPE_NONE;
> > > + break;
> >
> > IIRC you add dead code. IRQ framework never calls this if type is not set.
>
> Yes, but it is allowed to call
>
> irq_set_irq_type(irq, IRQ_TYPE_NONE);
>
> and the irq framework won't disallow it AFAICT.
That's true, but how you may end up in this callback with a such?
What the meaning of that call to the user?
...
> > > struct virtio_gpio_config {
> > > __le16 ngpio;
> > > __u8 padding[2];
> > > @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names {
> > > __u8 value[];
> > > };
> > >
> > > +/* Virtio GPIO IRQ Request / Response */
> > > +struct virtio_gpio_irq_request {
> > > + __le16 gpio;
> > > +};
> > > +
> > > +struct virtio_gpio_irq_response {
> > > + __u8 status;
> > > +};
> > >
> > I’m wondering if those above should be packed.
>
> You are talking about the newly added ones or the ones before ?
>
> In any case, they are all already packed (i.e. they have explicit
> padding wherever required) and properly aligned. Compiler won't add
> any other padding to them.
Is it only for 64-bit to 64-bit communications?
If there is a possibility to have 32-bit to 64-bit or vice versa
communication you have a problem.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V6] gpio: virtio: Add IRQ support
eature(vdev, VIRTIO_GPIO_F_IRQ)) {
> + vgpio->irq_lines = devm_kcalloc(dev, ngpio,
> + sizeof(*vgpio->irq_lines),
> + GFP_KERNEL);
One line?
> + if (!vgpio->irq_lines)
> + return -ENOMEM;
> +
> + /* The event comes from the outside so no parent handler */
> + vgpio->gc.irq.parent_handler= NULL;
> + vgpio->gc.irq.num_parents = 0;
> + vgpio->gc.irq.parents = NULL;
> + vgpio->gc.irq.default_type = IRQ_TYPE_NONE;
> + vgpio->gc.irq.handler = handle_level_irq;
> + vgpio->gc.irq.chip = _irq_chip;
> +
> + for (i = 0; i < ngpio; i++) {
> + vgpio->irq_lines[i].type =
> VIRTIO_GPIO_IRQ_TYPE_NONE;
> + vgpio->irq_lines[i].disabled = true;
> + vgpio->irq_lines[i].masked = true;
> + }
> +
> + mutex_init(>irq_lock);
> + raw_spin_lock_init(>eventq_lock);
> + }
> +
> ret = virtio_gpio_alloc_vqs(vgpio, vdev);
> if (ret)
> return ret;
> @@ -357,7 +653,13 @@ static const struct virtio_device_id id_table[] = {
> };
> MODULE_DEVICE_TABLE(virtio, id_table);
>
> +static const unsigned int features[] = {
> + VIRTIO_GPIO_F_IRQ,
> +};
> +
> static struct virtio_driver virtio_gpio_driver = {
> + .feature_table = features,
> + .feature_table_size = ARRAY_SIZE(features),
> .id_table = id_table,
> .probe = virtio_gpio_probe,
> .remove = virtio_gpio_remove,
> diff --git a/include/uapi/linux/virtio_gpio.h b/include/uapi/linux/virtio_
> gpio.h
> index 0445f905d8cc..d04af9c5f0de 100644
> --- a/include/uapi/linux/virtio_gpio.h
> +++ b/include/uapi/linux/virtio_gpio.h
> @@ -5,12 +5,16 @@
>
> #include
>
> +/* Virtio GPIO Feature bits */
> +#define VIRTIO_GPIO_F_IRQ 0
> +
> /* Virtio GPIO request types */
> #define VIRTIO_GPIO_MSG_GET_NAMES 0x0001
> #define VIRTIO_GPIO_MSG_GET_DIRECTION 0x0002
> #define VIRTIO_GPIO_MSG_SET_DIRECTION 0x0003
> #define VIRTIO_GPIO_MSG_GET_VALUE 0x0004
> #define VIRTIO_GPIO_MSG_SET_VALUE 0x0005
> +#define VIRTIO_GPIO_MSG_IRQ_TYPE 0x0006
>
> /* Possible values of the status field */
> #define VIRTIO_GPIO_STATUS_OK 0x0
> @@ -21,6 +25,14 @@
> #define VIRTIO_GPIO_DIRECTION_OUT 0x01
> #define VIRTIO_GPIO_DIRECTION_IN 0x02
>
> +/* Virtio GPIO IRQ types */
> +#define VIRTIO_GPIO_IRQ_TYPE_NONE 0x00
> +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING 0x01
> +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING 0x02
> +#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH 0x03
> +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH0x04
> +#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW 0x08
> +
> struct virtio_gpio_config {
> __le16 ngpio;
> __u8 padding[2];
> @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names {
> __u8 value[];
> };
>
> +/* Virtio GPIO IRQ Request / Response */
> +struct virtio_gpio_irq_request {
> + __le16 gpio;
> +};
> +
> +struct virtio_gpio_irq_response {
> + __u8 status;
> +};
>
>
I’m wondering if those above should be packed.
> +/* Possible values of the interrupt status field */
> +#define VIRTIO_GPIO_IRQ_STATUS_INVALID 0x0
> +#define VIRTIO_GPIO_IRQ_STATUS_VALID 0x1
> +
> #endif /* _LINUX_VIRTIO_GPIO_H */
> --
> 2.31.1.272.g89b43f80a514
>
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] gpio: aggregator: Add interrupt support
On Mon, Oct 4, 2021 at 3:45 PM Geert Uytterhoeven
wrote:
>
> Currently the GPIO Aggregator does not support interrupts. This means
> that kernel drivers going from a GPIO to an IRQ using gpiod_to_irq(),
> and userspace applications using line events do not work.
>
> Add interrupt support by providing a gpio_chip.to_irq() callback, which
> just calls into the parent GPIO controller.
>
> Note that this does not implement full interrupt controller (irq_chip)
> support, so using e.g. gpio-keys with "interrupts" instead of "gpios"
> still does not work.
...
> @@ -414,7 +421,8 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct
> device *dev,
> for (i = 0; i < ngpios; i++) {
> struct gpio_chip *parent = gpiod_to_chip(descs[i]);
>
> - dev_dbg(dev, "%u => gpio-%d\n", i, desc_to_gpio(descs[i]));
> + dev_dbg(dev, "%u => gpio %d irq %d\n", i,
> + desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
This is an unconditional call that will allocate the IRQ descriptor
even if we don't use it. Correct?
If so, I don't like this.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()
On Thu, Aug 12, 2021 at 09:34:00AM +0200, David Hildenbrand wrote: > On 12.08.21 09:14, Andy Shevchenko wrote: > > On Thursday, August 12, 2021, David Hildenbrand > <mailto:da...@redhat.com>> wrote: > > On 11.08.21 22:47, Andy Shevchenko wrote: > > On Wednesday, August 11, 2021, David Hildenbrand > > mailto:da...@redhat.com> > > <mailto:da...@redhat.com <mailto:da...@redhat.com>>> wrote: > > Yes, it’s like micro optimization. If you want your way I suggest then > > to add a macro > > > > #define for_each_iomem_resource_child() \ > > for (iomem_resource...) > > I think the only thing that really makes sense would be something like this > on top (not compiled yet): Makes sense to me, thanks, go for it! -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()
On Thursday, August 12, 2021, David Hildenbrand wrote:
> On 11.08.21 22:47, Andy Shevchenko wrote:
>
>>
>>
>> On Wednesday, August 11, 2021, David Hildenbrand > <mailto:da...@redhat.com>> wrote:
>>
>> Let's clean it up a bit, removing the unnecessary usage of r_next() by
>> next_resource(), and use next_range_resource() in case we are not
>> interested in a certain subtree.
>>
>> Signed-off-by: David Hildenbrand > <mailto:da...@redhat.com>>
>> ---
>> kernel/resource.c | 19 +++
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/resource.c b/kernel/resource.c
>> index 2938cf520ca3..ea853a075a83 100644
>> --- a/kernel/resource.c
>> +++ b/kernel/resource.c
>> @@ -1754,9 +1754,8 @@ static int strict_iomem_checks;
>>*/
>> bool iomem_is_exclusive(u64 addr)
>> {
>> - struct resource *p = _resource;
>> + struct resource *p;
>> bool err = false;
>> - loff_t l;
>> int size = PAGE_SIZE;
>>
>> if (!strict_iomem_checks)
>> @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
>> addr = addr & PAGE_MASK;
>>
>> read_lock(_lock);
>> - for (p = p->child; p ; p = r_next(NULL, p, )) {
>> + for (p = iomem_resource.child; p ;) {
>>
>>
> Hi Andy,
>
>
>> I consider the ordinal part of p initialization is slightly better and
>> done outside of read lock.
>>
>> Something like
>> p= _res...;
>> read lock
>> for (p = p->child; ...) {
>>
>
> Why should we care about doing that outside of the lock? That smells like
> a micro-optimization the compiler will most probably overwrite either way
> as the address of iomem_resource is just constant?
>
> Also, for me it's much more readable and compact if we perform a single
> initialization instead of two separate ones in this case.
>
> We're using the pattern I use in, find_next_iomem_res() and
> __region_intersects(), while we use the old pattern in
> iomem_map_sanity_check(), where we also use the same unnecessary r_next()
> call.
>
> I might just cleanup iomem_map_sanity_check() in a similar way.
>
>
Yes, it’s like micro optimization. If you want your way I suggest then to
add a macro
#define for_each_iomem_resource_child() \
for (iomem_resource...)
>
> Thanks,
>
> David / dhildenb
>
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 1/3] /dev/mem: disallow access to explicitly excluded system RAM regions
n p->sibling;
> }
>
> +static struct resource *next_resource_skip_children(struct resource *p)
> +{
> + while (!p->sibling && p->parent)
> + p = p->parent;
> + return p->sibling;
> +}
> +
> static void *r_next(struct seq_file *m, void *v, loff_t *pos)
> {
> struct resource *p = v;
> @@ -1700,6 +1707,41 @@ int iomem_map_sanity_check(resource_size_t addr,
> unsigned long size)
> return err;
> }
>
> +/*
> + * Check if a physical memory range is completely excluded from getting
> + * mapped/accessed via /dev/mem.
> + */
> +bool iomem_range_contains_excluded(u64 addr, u64 size)
> +{
> + const unsigned int flags = IORESOURCE_SYSTEM_RAM |
> IORESOURCE_EXCLUSIVE;
> + bool excluded = false;
> + struct resource *p;
> +
> + read_lock(_lock);
> + for (p = iomem_resource.child; p ;) {
Same comment as per patch 3.
> + if (p->start >= addr + size)
> + break;
> + if (p->end < addr) {
> + /* No need to consider children */
> + p = next_resource_skip_children(p);
> + continue;
> + }
> + /*
> +* A system RAM resource is excluded if
> IORESOURCE_EXCLUSIVE
> +* is set, even if not busy and even if we don't have
> strict
> +* checks enabled -- no ifs or buts.
> +*/
> + if ((p->flags & flags) == flags) {
> + excluded = true;
> + break;
> + }
> + p = next_resource(p);
> + }
> + read_unlock(_lock);
> +
> + return excluded;
> +}
> +
> #ifdef CONFIG_STRICT_DEVMEM
> static int strict_iomem_checks = 1;
> #else
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 5ddd575159fb..d0ce6e23a6db 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1780,7 +1780,9 @@ config STRICT_DEVMEM
> access to this is obviously disastrous, but specific access can
> be used by people debugging the kernel. Note that with PAT
> support
> enabled, even in this case there are restrictions on /dev/mem
> - use due to the cache aliasing requirements.
> + use due to the cache aliasing requirements. Further, some drivers
> + will still restrict access to some physical memory regions either
> + already used or to be used in the future as system RAM.
>
> If this option is switched on, and IO_STRICT_DEVMEM=n, the
> /dev/mem
> file only allows userspace access to PCI space and the BIOS code
> and
> --
> 2.31.1
>
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 3/3] kernel/resource: cleanup and optimize iomem_is_exclusive()
On Wednesday, August 11, 2021, David Hildenbrand wrote:
> Let's clean it up a bit, removing the unnecessary usage of r_next() by
> next_resource(), and use next_range_resource() in case we are not
> interested in a certain subtree.
>
> Signed-off-by: David Hildenbrand
> ---
> kernel/resource.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 2938cf520ca3..ea853a075a83 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1754,9 +1754,8 @@ static int strict_iomem_checks;
> */
> bool iomem_is_exclusive(u64 addr)
> {
> - struct resource *p = _resource;
> + struct resource *p;
> bool err = false;
> - loff_t l;
> int size = PAGE_SIZE;
>
> if (!strict_iomem_checks)
> @@ -1765,27 +1764,31 @@ bool iomem_is_exclusive(u64 addr)
> addr = addr & PAGE_MASK;
>
> read_lock(_lock);
> - for (p = p->child; p ; p = r_next(NULL, p, )) {
> + for (p = iomem_resource.child; p ;) {
I consider the ordinal part of p initialization is slightly better and done
outside of read lock.
Something like
p= _res...;
read lock
for (p = p->child; ...) {
> /*
> * We can probably skip the resources without
> * IORESOURCE_IO attribute?
> */
> if (p->start >= addr + size)
> break;
> - if (p->end < addr)
> + if (p->end < addr) {
> + /* No need to consider children */
> + p = next_resource_skip_children(p);
> continue;
> + }
> +
> /*
> * A resource is exclusive if IORESOURCE_EXCLUSIVE is set
> * or CONFIG_IO_STRICT_DEVMEM is enabled and the
> * resource is busy.
> */
> - if ((p->flags & IORESOURCE_BUSY) == 0)
> - continue;
> - if (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM)
> - || p->flags & IORESOURCE_EXCLUSIVE) {
> + if (p->flags & IORESOURCE_BUSY &&
> + (IS_ENABLED(CONFIG_IO_STRICT_DEVMEM) ||
> +p->flags & IORESOURCE_EXCLUSIVE)) {
> err = true;
> break;
> }
> + p = next_resource(p);
> }
> read_unlock(_lock);
>
> --
> 2.31.1
>
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 0/2] refactor the ringtest testing for ptr_ring
On Mon, Jul 5, 2021 at 9:45 PM Michael S. Tsirkin wrote: > > On Mon, Jul 05, 2021 at 09:36:26PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 05, 2021 at 02:26:32PM -0400, Michael S. Tsirkin wrote: > > > On Mon, Jul 05, 2021 at 08:06:50PM +0800, Yunsheng Lin wrote: > > > > On 2021/7/5 17:56, Andy Shevchenko wrote: > > > > > On Mon, Jul 05, 2021 at 11:57:33AM +0800, Yunsheng Lin wrote: > > > > >> tools/include/* have a lot of abstract layer for building > > > > >> kernel code from userspace, so reuse or add the abstract > > > > >> layer in tools/include/ to build the ptr_ring for ringtest > > > > >> testing. > > > > > > > > > > ... > > > > > > > > > >> create mode 100644 tools/include/asm/cache.h > > > > >> create mode 100644 tools/include/asm/processor.h > > > > >> create mode 100644 tools/include/generated/autoconf.h > > > > >> create mode 100644 tools/include/linux/align.h > > > > >> create mode 100644 tools/include/linux/cache.h > > > > >> create mode 100644 tools/include/linux/slab.h > > > > > > > > > > Maybe somebody can change this to be able to include in-tree headers > > > > > directly? > > > > > > > > If the above works, maybe the files in tools/include/* is not > > > > necessary any more, just use the in-tree headers to compile > > > > the user space app? > > > > > > > > Or I missed something here? > > > > > > why would it work? kernel headers outside of uapi are not > > > intended to be consumed by userspace. > > > > The problem here, that we are almost getting two copies of the headers, and > > tools are not in a good maintenance, so it's often desynchronized from the > > actual Linux headers. This will become more and more diverse if we keep same > > way of operation. So, I would rather NAK any new copies of the headers from > > include/ to tools/include. > > We already have the copies > yes they are not maintained well ... what's the plan then? > NAK won't help us improve the situation. I understand and the proposal is to leave only the files which are not the same (can we do kinda wrappers or so in tools/include rather than copying everything?). > I would say copies are kind of okay just make sure they are > built with kconfig. Then any breakage will be > detected. > > > > > > Besides above, had you tested this with `make O=...`? > > > > > > > > You are right, the generated/autoconf.h is in another directory > > > > with `make O=...`. > > > > > > > > Any nice idea to fix the above problem? -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 0/2] refactor the ringtest testing for ptr_ring
On Mon, Jul 05, 2021 at 02:26:32PM -0400, Michael S. Tsirkin wrote: > On Mon, Jul 05, 2021 at 08:06:50PM +0800, Yunsheng Lin wrote: > > On 2021/7/5 17:56, Andy Shevchenko wrote: > > > On Mon, Jul 05, 2021 at 11:57:33AM +0800, Yunsheng Lin wrote: > > >> tools/include/* have a lot of abstract layer for building > > >> kernel code from userspace, so reuse or add the abstract > > >> layer in tools/include/ to build the ptr_ring for ringtest > > >> testing. > > > > > > ... > > > > > >> create mode 100644 tools/include/asm/cache.h > > >> create mode 100644 tools/include/asm/processor.h > > >> create mode 100644 tools/include/generated/autoconf.h > > >> create mode 100644 tools/include/linux/align.h > > >> create mode 100644 tools/include/linux/cache.h > > >> create mode 100644 tools/include/linux/slab.h > > > > > > Maybe somebody can change this to be able to include in-tree headers > > > directly? > > > > If the above works, maybe the files in tools/include/* is not > > necessary any more, just use the in-tree headers to compile > > the user space app? > > > > Or I missed something here? > > why would it work? kernel headers outside of uapi are not > intended to be consumed by userspace. The problem here, that we are almost getting two copies of the headers, and tools are not in a good maintenance, so it's often desynchronized from the actual Linux headers. This will become more and more diverse if we keep same way of operation. So, I would rather NAK any new copies of the headers from include/ to tools/include. > > > Besides above, had you tested this with `make O=...`? > > > > You are right, the generated/autoconf.h is in another directory > > with `make O=...`. > > > > Any nice idea to fix the above problem? -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 0/2] refactor the ringtest testing for ptr_ring
On Mon, Jul 05, 2021 at 08:06:50PM +0800, Yunsheng Lin wrote: > On 2021/7/5 17:56, Andy Shevchenko wrote: > > On Mon, Jul 05, 2021 at 11:57:33AM +0800, Yunsheng Lin wrote: > >> tools/include/* have a lot of abstract layer for building > >> kernel code from userspace, so reuse or add the abstract > >> layer in tools/include/ to build the ptr_ring for ringtest > >> testing. > > > > ... > > > >> create mode 100644 tools/include/asm/cache.h > >> create mode 100644 tools/include/asm/processor.h > >> create mode 100644 tools/include/generated/autoconf.h > >> create mode 100644 tools/include/linux/align.h > >> create mode 100644 tools/include/linux/cache.h > >> create mode 100644 tools/include/linux/slab.h > > > > Maybe somebody can change this to be able to include in-tree headers > > directly? > > If the above works, maybe the files in tools/include/* is not > necessary any more, just use the in-tree headers to compile > the user space app? > > Or I missed something here? I don't know if it works or not or why that decision had been made to copy'n'paste headers (Yes, I know they have some modifications). Somebody needs to check that and see what can be done in order to avoid copying entire include into tools/include. > > Besides above, had you tested this with `make O=...`? > > You are right, the generated/autoconf.h is in another directory > with `make O=...`. > > Any nice idea to fix the above problem? No idea. But I consider breakage of O= is a show stopper. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 0/2] refactor the ringtest testing for ptr_ring
On Mon, Jul 05, 2021 at 11:57:33AM +0800, Yunsheng Lin wrote: > tools/include/* have a lot of abstract layer for building > kernel code from userspace, so reuse or add the abstract > layer in tools/include/ to build the ptr_ring for ringtest > testing. ... > create mode 100644 tools/include/asm/cache.h > create mode 100644 tools/include/asm/processor.h > create mode 100644 tools/include/generated/autoconf.h > create mode 100644 tools/include/linux/align.h > create mode 100644 tools/include/linux/cache.h > create mode 100644 tools/include/linux/slab.h Maybe somebody can change this to be able to include in-tree headers directly? Besides above, had you tested this with `make O=...`? -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v13] i2c: virtio: add a virtio i2c frontend driver
On Mon, Jul 5, 2021 at 11:56 AM Viresh Kumar wrote: > On 05-07-21, 11:45, Andy Shevchenko wrote: > > On Mon, Jul 5, 2021 at 11:03 AM Viresh Kumar > > wrote: > > > On 05-07-21, 14:53, Jie Deng wrote: > > > > > > +#include > > > > +#include > > > > > > Both of these need to be the uapi headers as Andy said earlier > > > > They are already since this header _is_ UAPI, > > Ahh, there is some tricky header inclusion there :) > > > what you are suggesting is gonna not work, > > Why ? Because we do not have "uapi" in the path in /usr/include on the real system where the linux-headers (or kernel-headers) package is installed. It's still possible that our installation hooks will remove that "uapi" from the headers, but I think it makes things too complicated. > > although it's correct for in-kernel users of UAPI > > headers. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v13] i2c: virtio: add a virtio i2c frontend driver
On Mon, Jul 5, 2021 at 11:03 AM Viresh Kumar wrote: > On 05-07-21, 14:53, Jie Deng wrote: > > +#include > > +#include > > Both of these need to be the uapi headers as Andy said earlier They are already since this header _is_ UAPI, what you are suggesting is gonna not work, although it's correct for in-kernel users of UAPI headers. > and they better > be in alphabetical order. I prefer that as well. > #include > #include -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v12] i2c: virtio: add a virtio i2c frontend driver
On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
...
> +static int virtio_i2c_complete_reqs(struct virtqueue *vq,
> + struct virtio_i2c_req *reqs,
> + struct i2c_msg *msgs, int nr,
> + bool fail)
> +{
> + struct virtio_i2c_req *req;
> + bool failed = fail;
> + unsigned int len;
> + int i, j = 0;
> +
> + for (i = 0; i < nr; i++) {
> + /* Detach the ith request from the vq */
> + req = virtqueue_get_buf(vq, );
> +
> + /*
> + * Condition (req && req == [i]) should always meet since
> + * we have total nr requests in the vq.
> + */
> + if (!failed && (WARN_ON(!(req && req == [i])) ||
> + (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
> + failed = true;
...and after failed is true, we are continuing the loop, why?
> + i2c_put_dma_safe_msg_buf(reqs[i].buf, [i], !failed);
> + if (!failed)
> + ++j;
Besides better to read j++ the j itself can be renamed to something more
verbose.
> + }
> + return (fail ? -ETIMEDOUT : j);
Redundant parentheses.
> +}
...
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret != num) {
> + virtio_i2c_complete_reqs(vq, reqs, msgs, ret, true);
Below you check the returned code, here is not.
> + ret = 0;
> + goto err_free;
> + }
> +
> + reinit_completion(>completion);
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(>completion, adap->timeout);
> + if (!time_left)
> + dev_err(>dev, "virtio i2c backend timeout.\n");
> +
> + ret = virtio_i2c_complete_reqs(vq, reqs, msgs, num, !time_left);
> +
> +err_free:
> + kfree(reqs);
> + return ret;
> +++ b/include/uapi/linux/virtio_i2c.h
> +#include
> +
> +/* The bit 0 of the @virtio_i2c_out_hdr.@flags, used to group the requests */
> +#define VIRTIO_I2C_FLAGS_FAIL_NEXT BIT(0)
It's _BITUL() or so from linux/const.h.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/const.h#L28
You may not use internal definitions in UAPI headers.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v11] i2c: virtio: add a virtio i2c frontend driver
On Thu, Jul 01, 2021 at 11:24:46AM +0800, Jie Deng wrote: > Add an I2C bus driver for virtio para-virtualization. > > The controller can be emulated by the backend driver in > any device model software by following the virtio protocol. > > The device specification can be found on > https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html. > > By following the specification, people may implement different > backend drivers to emulate different controllers according to > their needs. > - Use #ifdef CONFIG_PM_SLEEP to replace the "__maybe_unused". Why is that? -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v10] i2c: virtio: add a virtio i2c frontend driver
On Wed, Jun 30, 2021 at 09:55:49AM +0200, Arnd Bergmann wrote: > On Wed, Jun 30, 2021 at 9:51 AM Jie Deng wrote: ... > On a related note, we are apparently still missing the bit in the virtio bus > layer that fills in the dev->of_node pointer of the virtio device. Without > this, it is not actually possible to automatically probe i2c devices connected > to a virtio-i2c bus. The same problem came up again with the virtio-gpio > driver that suffers from the same issue. Don't we need to take care about fwnode handle as well? -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver
On Mon, Jun 14, 2021 at 11:08 AM Enrico Weigelt, metux IT consult wrote: > > On 11.06.21 10:01, Viresh Kumar wrote: > > > No, QEMU passes the raw messages to the backend daemon running in host > > userspace (which shares a socket with qemu). The backend understands > > the virtio/vhost protocols and so won't be required to change at all > > if we move from Qemu to something else. And that's what we (Linaro) > > are looking to do here with Project Stratos. > > Note that this is completely different from my approach that I've posted > in autumn last year. Viresh's protocol hasn't much in common with mine. That's why we have a thing called standard. And AFAIU virtio API/ABIs should be officially registered and standardized. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> On 01-03-21, 14:41, Jie Deng wrote:
> > +/**
> > + * struct virtio_i2c_req - the virtio I2C request structure
> > + * @out_hdr: the OUT header of the virtio I2C message
> > + * @write_buf: contains one I2C segment being written to the device
> > + * @read_buf: contains one I2C segment being read from the device
> > + * @in_hdr: the IN header of the virtio I2C message
> > + */
> > +struct virtio_i2c_req {
> > + struct virtio_i2c_out_hdr out_hdr;
> > + u8 *write_buf;
> > + u8 *read_buf;
> > + struct virtio_i2c_in_hdr in_hdr;
> > +};
>
> I am not able to appreciate the use of write/read bufs here as we
> aren't trying to read/write data in the same transaction. Why do we
> have two bufs here as well as in specs ?
I²C and SMBus support bidirectional transfers as well. I think two buffers is
the right thing to do.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 01, 2021 at 02:41:35PM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> The device specification can be found on
> https://lists.oasis-open.org/archives/virtio-comment/202101/msg8.html.
>
> By following the specification, people may implement different
> backend drivers to emulate different controllers according to
> their needs.
...
> + buf = kzalloc(msgs[i].len, GFP_KERNEL);
> + if (!buf)
> + break;
> +
> + if (msgs[i].flags & I2C_M_RD) {
kzalloc()
> + reqs[i].read_buf = buf;
> + sg_init_one(_buf, reqs[i].read_buf, msgs[i].len);
> + sgs[outcnt + incnt++] = _buf;
> + } else {
> + reqs[i].write_buf = buf;
> + memcpy(reqs[i].write_buf, msgs[i].buf, msgs[i].len);
kmemdup() ?
> + sg_init_one(_buf, reqs[i].write_buf, msgs[i].len);
> + sgs[outcnt++] = _buf;
> + }
...
> +
> +
One blank line is enough.
...
> + ret = virtio_i2c_send_reqs(vq, reqs, msgs, num);
> + if (ret == 0)
> + goto err_unlock_free;
> + else
Redundant.
> + nr = ret;
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] i2c: virtio: add a virtio i2c frontend driver
On Mon, Mar 01, 2021 at 02:09:25PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 05:24:41PM +0530, Viresh Kumar wrote:
> > On 01-03-21, 14:41, Jie Deng wrote:
> > > +/**
> > > + * struct virtio_i2c_req - the virtio I2C request structure
> > > + * @out_hdr: the OUT header of the virtio I2C message
> > > + * @write_buf: contains one I2C segment being written to the device
> > > + * @read_buf: contains one I2C segment being read from the device
> > > + * @in_hdr: the IN header of the virtio I2C message
> > > + */
> > > +struct virtio_i2c_req {
> > > + struct virtio_i2c_out_hdr out_hdr;
> > > + u8 *write_buf;
> > > + u8 *read_buf;
> > > + struct virtio_i2c_in_hdr in_hdr;
> > > +};
> >
> > I am not able to appreciate the use of write/read bufs here as we
> > aren't trying to read/write data in the same transaction. Why do we
> > have two bufs here as well as in specs ?
>
> I²C and SMBus support bidirectional transfers as well. I think two buffers is
> the right thing to do.
Strictly speaking "half duplex".
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] drivers: gpio: put virtual gpio device into their own submenu
On Thu, Dec 3, 2020 at 9:22 PM Enrico Weigelt, metux IT consult wrote: > > Since we already have a few virtual gpio devices, and more to come, gpio -> GPIO > this category deserves its own submenu. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 16/17] x86/ioapic: export a few functions and data structures via io_apic.h
On Wed, Dec 02, 2020 at 02:11:07PM +, Wei Liu wrote:
> On Wed, Nov 25, 2020 at 12:26:12PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 25, 2020 at 1:46 AM Wei Liu wrote:
> > >
> > > We are about to implement an irqchip for IO-APIC when Linux runs as root
> > > on Microsoft Hypervisor. At the same time we would like to reuse
> > > existing code as much as possible.
> > >
> > > Move mp_chip_data to io_apic.h and make a few helper functions
> > > non-static.
> >
> > > +struct mp_chip_data {
> > > + struct list_head irq_2_pin;
> > > + struct IO_APIC_route_entry entry;
> > > + int trigger;
> > > + int polarity;
> > > + u32 count;
> > > + bool isa_irq;
> > > +};
> >
> > Since I see only this patch I am puzzled why you need to have this in
> > the header?
> > Maybe a couple of words in the commit message to elaborate?
>
> Andy, does the following answer your question?
>
> "The chip_data stashed in IO-APIC's irq chip is mp_chip_data. The
> implementation of Microsoft Hypevisor's IO-APIC irqdomain would like to
> manipulate that data structure, so move it to io_apic.h as well."
At least it sheds some light, thanks.
> If that's good enough, I can add it to the commit message.
It's good for a starter, but I think you have to wait for what Thomas and other
related people can say.
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 10:39 PM James Bottomley wrote: > On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote: > > On Mon, Nov 23, 2020 at 4:58 PM James Bottomley > > wrote: ... > > But if we do the math, for an author, at even 1 minute per line > > change and assuming nothing can be automated at all, it would take 1 > > month of work. For maintainers, a couple of trivial lines is noise > > compared to many other patches. > > So you think a one line patch should take one minute to produce ... I > really don't think that's grounded in reality. I suppose a one line > patch only takes a minute to merge with b4 if no-one reviews or tests > it, but that's not really desirable. In my practice most of the one line patches were either to fix or to introduce quite interesting issues. 1 minute is 2-3 orders less than usually needed for such patches. That's why I don't like churn produced by people who often even didn't compile their useful contributions. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 16/17] x86/ioapic: export a few functions and data structures via io_apic.h
On Wed, Nov 25, 2020 at 1:46 AM Wei Liu wrote:
>
> We are about to implement an irqchip for IO-APIC when Linux runs as root
> on Microsoft Hypervisor. At the same time we would like to reuse
> existing code as much as possible.
>
> Move mp_chip_data to io_apic.h and make a few helper functions
> non-static.
> +struct mp_chip_data {
> + struct list_head irq_2_pin;
> + struct IO_APIC_route_entry entry;
> + int trigger;
> + int polarity;
> + u32 count;
> + bool isa_irq;
> +};
Since I see only this patch I am puzzled why you need to have this in
the header?
Maybe a couple of words in the commit message to elaborate?
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/8] slab: provide and use krealloc_array()
On Tue, Nov 3, 2020 at 12:13 PM Bartosz Golaszewski wrote: > On Tue, Nov 3, 2020 at 5:14 AM Joe Perches wrote: > > On Mon, 2020-11-02 at 16:20 +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski > Yeah so I had this concern for devm_krealloc() and even sent a patch > that extended it to honor __GFP_ZERO before I noticed that regular > krealloc() silently ignores __GFP_ZERO. I'm not sure if this is on > purpose. Maybe we should either make krealloc() honor __GFP_ZERO or > explicitly state in its documentation that it ignores it? And my voice here is to ignore for the same reasons: respect realloc(3) and making common sense with the idea of REallocating (capital letters on purpose). -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 8/8] dma-buf: use krealloc_array()
On Mon, Nov 02, 2020 at 04:20:37PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > Use the helper that checks for overflows internally instead of manually > calculating the size of the new array. ... > + nfences = krealloc_array(fences, i, > + sizeof(*fences), GFP_KERNEL); On 80 position is closing parenthesis, which, I think, makes it okay to put on one line. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] drm/virtio: Use UUID API for importing the UUID
There is import_uuid() function which imports u8 array to the uuid_t.
Use it instead of open coding variant.
This allows to hide the uuid_t internals.
Reviewed-by: David Stevens
Signed-off-by: Andy Shevchenko
---
v2: rebased on top of drm-misc-next (Gerd), added Rb tag (David)
drivers/gpu/drm/virtio/virtgpu_vq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 6434b9fb38a6..c1824f536936 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -1141,7 +1141,7 @@ static void virtio_gpu_cmd_resource_uuid_cb(struct
virtio_gpu_device *vgdev,
if (resp_type == VIRTIO_GPU_RESP_OK_RESOURCE_UUID &&
obj->uuid_state == STATE_INITIALIZING) {
- memcpy(>uuid.b, resp->uuid, sizeof(obj->uuid.b));
+ import_uuid(>uuid, resp->uuid);
obj->uuid_state = STATE_OK;
} else {
obj->uuid_state = STATE_ERR;
--
2.28.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v1] drm/virtio: Use UUID API for importing the UUID
There is import_uuid() function which imports u8 array to the uuid_t.
Use it instead of open coding variant.
This allows to hide the uuid_t internals.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/virtio/virtgpu_vq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 07945ca238e2..8944cc0bf8eb 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -1129,7 +1129,7 @@ static void virtio_gpu_cmd_resource_uuid_cb(struct
virtio_gpu_device *vgdev,
if (resp_type == VIRTIO_GPU_RESP_OK_RESOURCE_UUID &&
obj->uuid_state == UUID_INITIALIZING) {
- memcpy(>uuid.b, resp->uuid, sizeof(obj->uuid.b));
+ import_uuid(>uuid, resp->uuid);
obj->uuid_state = UUID_INITIALIZED;
} else {
obj->uuid_state = UUID_INITIALIZATION_FAILED;
--
2.28.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] i2c: virtio: add a virtio i2c frontend driver
On Tue, Sep 22, 2020 at 10:58:43AM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> This driver communicates with the backend driver through a
> virtio I2C message structure which includes following parts:
>
> - Header: i2c_msg addr, flags, len.
> - Data buffer: the pointer to the I2C msg data.
> - Status: the processing result from the backend.
>
> People may implement different backend drivers to emulate
> different controllers according to their needs. A backend
> example can be found in the device model of the open source
> project ACRN. For more information, please refer to
> https://projectacrn.org.
>
> The virtio device ID 34 is used for this I2C adpter since IDs
> before 34 have been reserved by other virtio devices.
>
> Co-developed-by: Conghui Chen
> Signed-off-by: Conghui Chen
> Signed-off-by: Jie Deng
> Reviewed-by: Shuo Liu
> Reviewed-by: Andy Shevchenko
> ---
> The device ID request:
> https://github.com/oasis-tcs/virtio-spec/issues/85
>
> The specification:
>
> https://lists.oasis-open.org/archives/virtio-comment/202009/msg00021.html
>
> Changes in v3:
> - Move the interface into uAPI according to Jason.
> - Fix issues reported by Dan Carpenter.
> - Fix typo reported by Randy.
>
> Changes in v2:
> - Addressed comments received from Michael, Andy and Jason.
>
> drivers/i2c/busses/Kconfig | 11 ++
> drivers/i2c/busses/Makefile | 3 +
> drivers/i2c/busses/i2c-virtio.c | 256
>
> include/uapi/linux/virtio_i2c.h | 31 +
> include/uapi/linux/virtio_ids.h | 1 +
> 5 files changed, 302 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-virtio.c
> create mode 100644 include/uapi/linux/virtio_i2c.h
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 293e7a0..f2f6543 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -21,6 +21,17 @@ config I2C_ALI1535
> This driver can also be built as a module. If so, the module
> will be called i2c-ali1535.
>
> +config I2C_VIRTIO
> + tristate "Virtio I2C Adapter"
> + depends on VIRTIO
> + help
> + If you say yes to this option, support will be included for the virtio
> + I2C adapter driver. The hardware can be emulated by any device model
> + software according to the virtio protocol.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-virtio.
> +
> config I2C_ALI1563
> tristate "ALI 1563"
> depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 19aff0e..821acfa 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -6,6 +6,9 @@
> # ACPI drivers
> obj-$(CONFIG_I2C_SCMI) += i2c-scmi.o
>
> +# VIRTIO I2C host controller driver
> +obj-$(CONFIG_I2C_VIRTIO) += i2c-virtio.o
> +
> # PC SMBus host controller drivers
> obj-$(CONFIG_I2C_ALI1535)+= i2c-ali1535.o
> obj-$(CONFIG_I2C_ALI1563)+= i2c-ali1563.o
> diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c
> new file mode 100644
> index 000..48fd780
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-virtio.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Virtio I2C Bus Driver
> + *
> + * Copyright (c) 2020 Intel Corporation. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +/**
> + * struct virtio_i2c_msg - the virtio I2C message structure
> + * @hdr: the virtio I2C message header
> + * @buf: virtio I2C message data buffer
> + * @status: the processing result from the backend
> + */
> +struct virtio_i2c_msg {
> + struct virtio_i2c_hdr hdr;
> + u8 *buf;
> + u8 status;
> +};
> +
> +/**
> + * struct virtio_i2c - virtio I2C data
> + * @vdev: virtio device for this controller
> + * @completion: completion of virtio I2C message
> + * @vmsg: the virtio I2C message for communication
> + * @adap: I2C adapter for this controller
> + * @i2c_lock: lock for virtqueue processing
> + * @vq: the virtio virtqueue for communication
> + */
> +struct virtio_i2c {
> + struct virtio_device *vdev;
> + struct completion completion;
> +
Re: [PATCH v2] i2c: virtio: add a virtio i2c frontend driver
On Mon, Sep 14, 2020 at 06:47:27PM +0300, Dan Carpenter wrote:
> On Mon, Sep 14, 2020 at 06:24:55PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 14, 2020 at 05:48:07PM +0300, Dan Carpenter wrote:
> > > Hi Jie,
> > >
> > > url:
> > > https://github.com/0day-ci/linux/commits/Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20200911-115013
> > >
> > > base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
> > > i2c/for-next
> > > config: parisc-randconfig-m031-20200913 (attached as .config)
> > > compiler: hppa-linux-gcc (GCC) 9.3.0
> > >
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot
> > > Reported-by: Dan Carpenter
> > >
> > > smatch warnings:
> > > drivers/i2c/busses/i2c-virtio.c:160 virtio_i2c_xfer() error: we
> > > previously assumed 'vmsg' could be null (see line 137)
> > >
> >
> > It's quite possible a false positive. Look at 122. But I agree that
> > for-loop is
> > not the best for such things to understand. Perhaps switching to do {}
> > while ()
> > will make it better.
> >
>
> Smatch is assuming that virtqueue_get_buf() can return NULL on the last
> iteration through the loop.
I see now. Thanks.
> > > #
> > > https://github.com/0day-ci/linux/commit/0a54ec771966748fcbc86256b830b5f786168b7d
> > >
> > > git remote add linux-review https://github.com/0day-ci/linux
> > > git fetch --no-tags linux-review
> > > Jie-Deng/i2c-virtio-add-a-virtio-i2c-frontend-driver/20200911-115013
> > > git checkout 0a54ec771966748fcbc86256b830b5f786168b7d
> > > vim +/vmsg +160 drivers/i2c/busses/i2c-virtio.c
> > >
> > > 0a54ec77196674 Jie Deng 2020-09-11 109 static int
> > > virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> > > 0a54ec77196674 Jie Deng 2020-09-11 110 {
> > > 0a54ec77196674 Jie Deng 2020-09-11 111 struct virtio_i2c *vi =
> > > i2c_get_adapdata(adap);
> > > 0a54ec77196674 Jie Deng 2020-09-11 112 struct virtqueue *vq = vi->vq;
> > > 0a54ec77196674 Jie Deng 2020-09-11 113 struct virtio_i2c_msg *vmsg;
> > > 0a54ec77196674 Jie Deng 2020-09-11 114 unsigned long time_left;
> > > 0a54ec77196674 Jie Deng 2020-09-11 115 int len, i, ret = 0;
> > > 0a54ec77196674 Jie Deng 2020-09-11 116
> > > 0a54ec77196674 Jie Deng 2020-09-11 117 mutex_lock(>i2c_lock);
> > > 0a54ec77196674 Jie Deng 2020-09-11 118 vmsg = >vmsg;
> > > 0a54ec77196674 Jie Deng 2020-09-11 119 vmsg->buf = NULL;
> > > 0a54ec77196674 Jie Deng 2020-09-11 120
> > > 0a54ec77196674 Jie Deng 2020-09-11 121 for (i = 0; i < num; i++) {
> > > 0a54ec77196674 Jie Deng 2020-09-11 122 ret =
> > > virtio_i2c_add_msg(vq, vmsg, [i]);
> > > 0a54ec77196674 Jie Deng 2020-09-11 123 if (ret) {
> > > 0a54ec77196674 Jie Deng 2020-09-11 124
> > > dev_err(>dev, "failed to add msg[%d] to virtqueue.\n", i);
> > > 0a54ec77196674 Jie Deng 2020-09-11 125 break;
> > > 0a54ec77196674 Jie Deng 2020-09-11 126 }
> > > 0a54ec77196674 Jie Deng 2020-09-11 127
> > > 0a54ec77196674 Jie Deng 2020-09-11 128 virtqueue_kick(vq);
> > > 0a54ec77196674 Jie Deng 2020-09-11 129
> > > 0a54ec77196674 Jie Deng 2020-09-11 130 time_left =
> > > wait_for_completion_timeout(>completion, adap->timeout);
> > > 0a54ec77196674 Jie Deng 2020-09-11 131 if (!time_left) {
> > > 0a54ec77196674 Jie Deng 2020-09-11 132
> > > dev_err(>dev, "msg[%d]: addr=0x%x timeout.\n", i, msgs[i].addr);
> > > 0a54ec77196674 Jie Deng 2020-09-11 133 break;
> > > 0a54ec77196674 Jie Deng 2020-09-11 134 }
> > > 0a54ec77196674 Jie Deng 2020-09-11 135
> > > 0a54ec77196674 Jie Deng 2020-09-11 136 vmsg = (struct
> > > virtio_i2c_msg *)virtqueue_get_buf(vq, );
> > > 0a54ec77196674 Jie Deng 2020-09-11 @137 if (vmsg) {
> > >
> > > Check for NULL.
> > >
> > > 0a54ec77196674 Jie Deng 2020-09-11 138 /* vmsg should
> > > point to the same address with >vmsg */
> > > 0a54ec77196674 Jie Deng 2020-09-11 139 if (vmsg !=
> > > >vmsg) {
> > > 0a54ec77196
Re: [PATCH v2] i2c: virtio: add a virtio i2c frontend driver
Deng 2020-09-11 149 if > ((msgs[i].flags & I2C_M_RD) && msgs[i].len) > 0a54ec77196674 Jie Deng 2020-09-11 150 > memcpy(msgs[i].buf, vmsg->buf, msgs[i].len); > 0a54ec77196674 Jie Deng 2020-09-11 151 > 0a54ec77196674 Jie Deng 2020-09-11 152 > kfree(vmsg->buf); > 0a54ec77196674 Jie Deng 2020-09-11 153 vmsg->buf = > NULL; > 0a54ec77196674 Jie Deng 2020-09-11 154 } > 0a54ec77196674 Jie Deng 2020-09-11 155 > 0a54ec77196674 Jie Deng 2020-09-11 156 > reinit_completion(>completion); > 0a54ec77196674 Jie Deng 2020-09-11 157 } > 0a54ec77196674 Jie Deng 2020-09-11 158 > 0a54ec77196674 Jie Deng 2020-09-11 159 mutex_unlock(>i2c_lock); > 0a54ec77196674 Jie Deng 2020-09-11 @160 kfree(vmsg->buf); > ^ > Unchecked dereference. > > 0a54ec77196674 Jie Deng 2020-09-11 161 return ((ret < 0) ? ret : i); > 0a54ec77196674 Jie Deng 2020-09-11 162 } > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] i2c: virtio: add a virtio i2c frontend driver
On Thu, Sep 03, 2020 at 01:34:45PM +0800, Jie Deng wrote:
> Add an I2C bus driver for virtio para-virtualization.
>
> The controller can be emulated by the backend driver in
> any device model software by following the virtio protocol.
>
> This driver communicates with the backend driver through a
> virtio I2C message structure which includes following parts:
>
> - Header: i2c_msg addr, flags, len.
> - Data buffer: the pointer to the i2c msg data.
> - Status: the processing result from the backend.
>
> People may implement different backend drivers to emulate
> different controllers according to their needs. A backend
> example can be found in the device model of the open source
> project ACRN. For more information, please refer to
> https://projectacrn.org.
>
> The virtio device ID 34 is used for this I2C adpter since IDs
> before 34 have been reserved by other virtio devices.
Seems it's slightly different version to what I have reviewed internally.
My comments below. (I admit that some of them maybe new)
...
> +/**
> + * struct virtio_i2c_hdr - the virtio I2C message header structure
> + * @addr: i2c_msg addr, the slave address
> + * @flags: i2c_msg flags
> + * @len: i2c_msg len
> + */
> +struct virtio_i2c_hdr {
> + __virtio16 addr;
> + __virtio16 flags;
> + __virtio16 len;
> +} __packed;
As Misha noticed and somewhere I saw 0-day reports these should be carefully
taken care of.
...
> +static int virtio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> int num)
> +{
> + struct virtio_i2c *vi = i2c_get_adapdata(adap);
> + struct virtio_i2c_msg *vmsg_o, *vmsg_i;
> + struct virtqueue *vq = vi->vq;
> + unsigned long time_left;
> + int len, i, ret = 0;
> +
> + vmsg_o = kzalloc(sizeof(*vmsg_o), GFP_KERNEL);
> + if (!vmsg_o)
> + return -ENOMEM;
> +
> + mutex_lock(>i2c_lock);
> + vmsg_o->buf = NULL;
> + for (i = 0; i < num; i++) {
> + ret = virtio_i2c_add_msg(vq, vmsg_o, [i]);
> + if (ret) {
> + dev_err(>dev, "failed to add msg[%d] to
> virtqueue.\n", i);
> + goto err_unlock_free;
break;
> + }
> +
> + virtqueue_kick(vq);
> +
> + time_left = wait_for_completion_timeout(>completion,
> adap->timeout);
> + if (!time_left) {
> + dev_err(>dev, "msg[%d]: addr=0x%x timeout.\n", i,
> msgs[i].addr);
> + ret = i;
> + goto err_unlock_free;
break;
And so on.
> + }
> +
> + vmsg_i = (struct virtio_i2c_msg *)virtqueue_get_buf(vq, );
> + if (vmsg_i) {
> + /* vmsg_i should point to the same address with vmsg_o
> */
> + if (vmsg_i != vmsg_o) {
> + dev_err(>dev, "msg[%d]: addr=0x%x
> virtqueue error.\n",
> + i, vmsg_i->hdr.addr);
> + ret = i;
> + goto err_unlock_free;
> + }
> + if (vmsg_i->status != VIRTIO_I2C_MSG_OK) {
> + dev_err(>dev, "msg[%d]: addr=0x%x
> error=%d.\n",
> + i, vmsg_i->hdr.addr, vmsg_i->status);
> + ret = i;
> + goto err_unlock_free;
> + }
> + if ((vmsg_i->hdr.flags & I2C_M_RD) && vmsg_i->hdr.len)
> + memcpy(msgs[i].buf, vmsg_i->buf,
> vmsg_i->hdr.len);
> +
> + kfree(vmsg_i->buf);
> + vmsg_i->buf = NULL;
> + }
> + reinit_completion(>completion);
> + }
> + if (i == num)
> + ret = num;
And this conditional seems a dup of the for-loop successfully iterating over
entire queue.
> +err_unlock_free:
Redundant.
> + mutex_unlock(>i2c_lock);
> + kfree(vmsg_o->buf);
> + kfree(vmsg_o);
> + return ret;
> +}
...
> + vi->adap.timeout = HZ / 10;
+ Blank line.
> + ret = i2c_add_adapter(>adap);
> + if (ret) {
> + dev_err(>dev, "failed to add virtio-i2c adapter.\n");
> + virtio_i2c_del_vqs(vdev);
Usually we do clean up followed by message.
> + }
> +
> + return ret;
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1] drm/bochs: fix an issue of ioremap() leak
On Wed, Apr 22, 2020 at 4:52 PM Dejin Zheng wrote:
>
> On Tue, Apr 21, 2020 at 08:24:24PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 21, 2020 at 7:45 PM Dejin Zheng wrote:
> > >
> > > It forgot to call bochs_hw_fini() to release related resources when
> > > bochs_pci_probe() fail. eg: io virtual address get by ioremap().
> >
> > Good start, although I think the best is to switch this driver to use
> > pcim_*() functions and drop tons of legacy code.
> >
> Andy, thanks for your encouragement, I think we might be able to fix this
> issue first, after that, drop tons of legacy code by pcim_*() functions.
> Do you think it is ok?
It's really up to maintainer. I'm not the one here.
> > > Fixes: 81da8c3b8d3df6 ("drm/bochs: add drm_driver.release callback.")
> > > CC: Andy Shevchenko
> > > Signed-off-by: Dejin Zheng
> > > ---
> > > drivers/gpu/drm/bochs/bochs_drv.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/gpu/drm/bochs/bochs_drv.c
> > > b/drivers/gpu/drm/bochs/bochs_drv.c
> > > index addb0568c1af..210a60135c8a 100644
> > > --- a/drivers/gpu/drm/bochs/bochs_drv.c
> > > +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> > > @@ -138,6 +138,7 @@ static int bochs_pci_probe(struct pci_dev *pdev,
> > > return ret;
> > >
> > > err_unload:
> > > + bochs_hw_fini(dev);
> > > bochs_unload(dev);
> > > err_free_dev:
> > > drm_dev_put(dev);
> > > --
> > > 2.25.0
> > >
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1] drm/bochs: fix an issue of ioremap() leak
On Tue, Apr 21, 2020 at 7:45 PM Dejin Zheng wrote:
>
> It forgot to call bochs_hw_fini() to release related resources when
> bochs_pci_probe() fail. eg: io virtual address get by ioremap().
Good start, although I think the best is to switch this driver to use
pcim_*() functions and drop tons of legacy code.
> Fixes: 81da8c3b8d3df6 ("drm/bochs: add drm_driver.release callback.")
> CC: Andy Shevchenko
> Signed-off-by: Dejin Zheng
> ---
> drivers/gpu/drm/bochs/bochs_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c
> b/drivers/gpu/drm/bochs/bochs_drv.c
> index addb0568c1af..210a60135c8a 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -138,6 +138,7 @@ static int bochs_pci_probe(struct pci_dev *pdev,
> return ret;
>
> err_unload:
> + bochs_hw_fini(dev);
> bochs_unload(dev);
> err_free_dev:
> drm_dev_put(dev);
> --
> 2.25.0
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 3/7] x86/jailhouse: Enable PCI mmconfig access in inmates
On Sun, Mar 4, 2018 at 8:31 PM, Jan Kiszka <jan.kis...@siemens.com> wrote:
> From: Otavio Pontes <otavio.pon...@intel.com>
>
> Use the PCI mmconfig base address exported by jailhouse in boot
> parameters in order to access the memory mapped PCI configuration space.
>
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>
> Signed-off-by: Otavio Pontes <otavio.pon...@intel.com>
> [Jan: rebased, fixed !CONFIG_PCI_MMCONFIG, used pcibios_last_bus]
> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
> ---
> arch/x86/include/asm/pci_x86.h | 2 ++
> arch/x86/kernel/jailhouse.c| 8
> arch/x86/pci/mmconfig-shared.c | 4 ++--
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
> index eb66fa9cd0fc..959d618dbb17 100644
> --- a/arch/x86/include/asm/pci_x86.h
> +++ b/arch/x86/include/asm/pci_x86.h
> @@ -151,6 +151,8 @@ extern int pci_mmconfig_insert(struct device *dev, u16
> seg, u8 start, u8 end,
>phys_addr_t addr);
> extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);
> extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
> +extern struct pci_mmcfg_region *__init pci_mmconfig_add(int segment, int
> start,
> + int end, u64 addr);
>
> extern struct list_head pci_mmcfg_list;
>
> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
> index b68fd895235a..fa183a131edc 100644
> --- a/arch/x86/kernel/jailhouse.c
> +++ b/arch/x86/kernel/jailhouse.c
> @@ -124,6 +124,14 @@ static int __init jailhouse_pci_arch_init(void)
> if (pcibios_last_bus < 0)
> pcibios_last_bus = 0xff;
>
> +#ifdef CONFIG_PCI_MMCONFIG
> + if (setup_data.pci_mmconfig_base) {
> + pci_mmconfig_add(0, 0, pcibios_last_bus,
> +setup_data.pci_mmconfig_base);
> + pci_mmcfg_arch_init();
> + }
> +#endif
> +
> return 0;
> }
>
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 96684d0adcf9..0e590272366b 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -94,8 +94,8 @@ static struct pci_mmcfg_region *pci_mmconfig_alloc(int
> segment, int start,
> return new;
> }
>
> -static struct pci_mmcfg_region *__init pci_mmconfig_add(int segment, int
> start,
> - int end, u64 addr)
> +struct pci_mmcfg_region *__init pci_mmconfig_add(int segment, int start,
> +int end, u64 addr)
> {
> struct pci_mmcfg_region *new;
>
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 4/6] x86: Consolidate PCI_MMCONFIG configs
On Thu, Mar 1, 2018 at 5:13 PM, Bjorn Helgaas <helg...@kernel.org> wrote:
> On Thu, Mar 01, 2018 at 06:40:47AM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kis...@siemens.com>
>>
>> Since e279b6c1d329 ("x86: start unification of arch/x86/Kconfig.*"), we
>> have two PCI_MMCONFIG entries, one from the original i386 and another
>> from x86_64. This consolidates both entries into a single one.
>>
>> The logic for x86_32, where this option was not under user control,
>> remains identical. On x86_64, PCI_MMCONFIG becomes additionally
>> configurable for SFI systems even if ACPI was disabled. This just
>> simplifies the logic without restricting the configurability in any way.
>
> Thanks for mentioning this difference. It's probably trivial, but if
> you have any other reason to respin this series, I would split this
> into two patches:
>
> - allow PCI_MMCONFIG on x86_64 with SFI
> - consolidate PCI_MMCONFIG with no logical change at all
>
>> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
>
> But either way,
>
> Acked-by: Bjorn Helgaas <bhelg...@google.com>
>
If you going to respin I would suggest one more trivia
Split long depends on to two lines, like
- depends on PCI && (X86_64 || (PCI_GODIRECT || PCI_GOANY ||
PCI_GOOLPC || PCI_GOMMCONFIG))
+ depends on PCI
+ depends on X86_64 || (PCI_GODIRECT || PCI_GOANY || PCI_GOOLPC
|| PCI_GOMMCONFIG)
...
depends on PCI && (ACPI || SFI) && (X86_64 || (PCI_GOANY ||
PCI_GOMMCONFIG))
+ depends on PCI && (ACPI || SFI)
+ depends on X86_64 || (PCI_GOANY || PCI_GOMMCONFIG)
(perhaps in a separate change)
>> ---
>> arch/x86/Kconfig | 9 +++--
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index eb7f43f23521..aef9d67ac186 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2641,8 +2641,9 @@ config PCI_DIRECT
>> depends on PCI && (X86_64 || (PCI_GODIRECT || PCI_GOANY || PCI_GOOLPC
>> || PCI_GOMMCONFIG))
>>
>> config PCI_MMCONFIG
>> - def_bool y
>> - depends on X86_32 && PCI && (ACPI || SFI) && (PCI_GOMMCONFIG ||
>> PCI_GOANY)
>> + bool "Support mmconfig PCI config space access" if X86_64
>> + default y
>> + depends on PCI && (ACPI || SFI) && (X86_64 || (PCI_GOANY ||
>> PCI_GOMMCONFIG))
>>
>> config PCI_OLPC
>> def_bool y
>> @@ -2657,10 +2658,6 @@ config PCI_DOMAINS
>> def_bool y
>> depends on PCI
>>
>> -config PCI_MMCONFIG
>> - bool "Support mmconfig PCI config space access"
>> - depends on X86_64 && PCI && ACPI
>> -
>> config PCI_CNB20LE_QUIRK
>> bool "Read CNB20LE Host Bridge Windows" if EXPERT
>> depends on PCI
>> --
>> 2.13.6
>>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 3/6] x86/jailhouse: Enable PCI mmconfig access in inmates
On Thu, Mar 1, 2018 at 7:40 AM, Jan Kiszka <jan.kis...@siemens.com> wrote:
> Use the PCI mmconfig base address exported by jailhouse in boot
> parameters in order to access the memory mapped PCI configuration space.
> --- a/arch/x86/kernel/jailhouse.c
> +++ b/arch/x86/kernel/jailhouse.c
> @@ -124,6 +124,13 @@ static int __init jailhouse_pci_arch_init(void)
> if (pcibios_last_bus < 0)
> pcibios_last_bus = 0xff;
>
> +#ifdef CONFIG_PCI_MMCONFIG
> + if (setup_data.pci_mmconfig_base) {
> + pci_mmconfig_add(0, 0, 0xff, setup_data.pci_mmconfig_base);
Hmm... Shouldn't be pcibios_last_bus instead of 0xff?
> + pci_mmcfg_arch_init();
> + }
> +#endif
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 4/6] x86: Consolidate PCI_MMCONFIG configs
On Thu, Mar 1, 2018 at 7:40 AM, Jan Kiszka <jan.kis...@siemens.com> wrote:
> From: Jan Kiszka <jan.kis...@siemens.com>
>
> Since e279b6c1d329 ("x86: start unification of arch/x86/Kconfig.*"), we
> have two PCI_MMCONFIG entries, one from the original i386 and another
> from x86_64. This consolidates both entries into a single one.
>
> The logic for x86_32, where this option was not under user control,
> remains identical. On x86_64, PCI_MMCONFIG becomes additionally
> configurable for SFI systems even if ACPI was disabled. This just
> simplifies the logic without restricting the configurability in any way.
>
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>
> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
> ---
> arch/x86/Kconfig | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index eb7f43f23521..aef9d67ac186 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2641,8 +2641,9 @@ config PCI_DIRECT
> depends on PCI && (X86_64 || (PCI_GODIRECT || PCI_GOANY || PCI_GOOLPC
> || PCI_GOMMCONFIG))
>
> config PCI_MMCONFIG
> - def_bool y
> - depends on X86_32 && PCI && (ACPI || SFI) && (PCI_GOMMCONFIG ||
> PCI_GOANY)
> + bool "Support mmconfig PCI config space access" if X86_64
> + default y
> + depends on PCI && (ACPI || SFI) && (X86_64 || (PCI_GOANY ||
> PCI_GOMMCONFIG))
>
> config PCI_OLPC
> def_bool y
> @@ -2657,10 +2658,6 @@ config PCI_DOMAINS
> def_bool y
> depends on PCI
>
> -config PCI_MMCONFIG
> - bool "Support mmconfig PCI config space access"
> - depends on X86_64 && PCI && ACPI
> -
> config PCI_CNB20LE_QUIRK
> bool "Read CNB20LE Host Bridge Windows" if EXPERT
> depends on PCI
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/6] PCI: Scan all functions when running over Jailhouse
On Thu, Mar 1, 2018 at 7:40 AM, Jan Kiszka <jan.kis...@siemens.com> wrote:
> From: Jan Kiszka <jan.kis...@siemens.com>
>
> Per PCIe r4.0, sec 7.5.1.1.9, multi-function devices are required to
> have a function 0. Therefore, Linux scans for devices at function 0
> (devfn 0/8/16/...) and only scans for other functions if function 0
> has its Multi-Function Device bit set or ARI or SR-IOV indicate
> there are more functions.
>
> The Jailhouse hypervisor may pass individual functions of a
> multi-function device to a guest without passing function 0, which
> means a Linux guest won't find them.
>
> Change Linux PCI probing so it scans all function numbers when
> running as a guest over Jailhouse.
>
> This is technically prohibited by the spec, so it is possible that
> PCI devices without the Multi-Function Device bit set may have
> unexpected behavior in response to this probe.
>
> Derived from original patch by Benedikt Spranger.
>
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>
One nit below.
> CC: Benedikt Spranger <b.spran...@linutronix.de>
> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
> Acked-by: Bjorn Helgaas <bhelg...@google.com>
> ---
> arch/x86/pci/legacy.c | 4 +++-
> drivers/pci/probe.c | 22 +++---
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/pci/legacy.c b/arch/x86/pci/legacy.c
> index 1cb01abcb1be..dfbe6ac38830 100644
> --- a/arch/x86/pci/legacy.c
> +++ b/arch/x86/pci/legacy.c
> @@ -4,6 +4,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /*
> @@ -34,13 +35,14 @@ int __init pci_legacy_init(void)
>
> void pcibios_scan_specific_bus(int busn)
> {
> + int stride = jailhouse_paravirt() ? 1 : 8;
> int devfn;
> u32 l;
>
> if (pci_find_bus(0, busn))
> return;
>
> - for (devfn = 0; devfn < 256; devfn += 8) {
> + for (devfn = 0; devfn < 256; devfn += stride) {
> if (!raw_pci_read(0, busn, devfn, PCI_VENDOR_ID, 2, ) &&
> l != 0x && l != 0x) {
> DBG("Found device at %02x:%02x [%04x]\n", busn,
> devfn, l);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ef5377438a1e..da22d6d216f8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include "pci.h"
> @@ -2518,14 +2519,29 @@ static unsigned int pci_scan_child_bus_extend(struct
> pci_bus *bus,
> {
> unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0;
> unsigned int start = bus->busn_res.start;
> - unsigned int devfn, cmax, max = start;
> + unsigned int devfn, fn, cmax, max = start;
> struct pci_dev *dev;
> + int nr_devs;
>
> dev_dbg(>dev, "scanning bus\n");
>
> /* Go find them, Rover! */
> - for (devfn = 0; devfn < 0x100; devfn += 8)
> - pci_scan_slot(bus, devfn);
> + for (devfn = 0; devfn < 0x100; devfn += 8) {
Since you touch this line perhaps make sense to unify with above, i.e.
0x100 -> 256 ?
> + nr_devs = pci_scan_slot(bus, devfn);
> +
> + /*
> +* The Jailhouse hypervisor may pass individual functions of a
> +* multi-function device to a guest without passing function
> 0.
> +* Look for them as well.
> +*/
> + if (jailhouse_paravirt() && nr_devs == 0) {
> + for (fn = 1; fn < 8; fn++) {
> + dev = pci_scan_single_device(bus, devfn + fn);
> + if (dev)
> + dev->multifunction = 1;
> + }
> + }
> + }
>
> /* Reserve buses for SR-IOV capability */
> used_buses = pci_iov_bus_range(bus);
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 4/6] x86: Consolidate PCI_MMCONFIG configs
On Wed, Feb 28, 2018 at 8:34 AM, Jan Kiszka <jan.kis...@siemens.com> wrote: > From: Jan Kiszka <jan.kis...@siemens.com> > > Not sure if those two worked by design or just by chance so far. In any > case, it's at least cleaner and clearer to express this in a single > config statement. I would add a reference to the commit which brought that in the first place. > > Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> > --- > arch/x86/Kconfig | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index eb7f43f23521..63e85e7da12e 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2641,8 +2641,9 @@ config PCI_DIRECT > depends on PCI && (X86_64 || (PCI_GODIRECT || PCI_GOANY || PCI_GOOLPC > || PCI_GOMMCONFIG)) > > config PCI_MMCONFIG > - def_bool y > - depends on X86_32 && PCI && (ACPI || SFI) && (PCI_GOMMCONFIG || > PCI_GOANY) > + bool "Support mmconfig PCI config space access" if X86_64 > + default y > + depends on PCI && (ACPI || SFI) && (PCI_GOMMCONFIG || PCI_GOANY || > X86_64) Looking to the above context I would rather put it like depends on PCI && (ACPI || SFI) && (X86_64 || (PCI_GOANY || PCI_GOMMCONFIG)) -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] x86: Consolidate PCI_MMCONFIG configs
On Tue, Feb 27, 2018 at 9:19 AM, Jan Kiszka <jan.kis...@siemens.com> wrote: > On 2018-01-28 18:26, Andy Shevchenko wrote: >> On Mon, Jan 22, 2018 at 8:12 AM, Jan Kiszka <jan.kis...@siemens.com> wrote: >>> From: Jan Kiszka <jan.kis...@siemens.com> >>> >>> Not sure if those two worked by design or just by chance so far. In any >>> case, it's at least cleaner and clearer to express this in a single >>> config statement. >> >> Congrats! You found by the way a bug in >> >> commit e279b6c1d329e50b766bce96aacc197eae8a053b >> Author: Sam Ravnborg <s...@ravnborg.org> >> Date: Tue Nov 6 20:41:05 2007 +0100 >> >>x86: start unification of arch/x86/Kconfig.* >> >> ...and proper fix seems to split PCI stuff to common + X86_32 only + X86_64 >> only >> > > Hmm, is that a change request on this patch? >From my side it's a suggestion. Better wait for the answer from x86 maintainers. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/6] pci: Scan all functions when probing while running over Jailhouse
On Mon, Jan 22, 2018 at 8:12 AM, Jan Kiszka <jan.kis...@siemens.com> wrote: > #include > #include > #include > +#include Keep it in order? > #include > #include > #include > +#include Ditto. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v21 1/5] xbitmap: Introduce xbitmap
On Fri, Feb 16, 2018 at 8:30 PM, Matthew Wilcox <wi...@infradead.org> wrote: > On Fri, Feb 16, 2018 at 07:44:50PM +0200, Andy Shevchenko wrote: >> On Tue, Jan 9, 2018 at 1:10 PM, Wei Wang <wei.w.w...@intel.com> wrote: >> > From: Matthew Wilcox <mawil...@microsoft.com> >> > >> > The eXtensible Bitmap is a sparse bitmap representation which is >> > efficient for set bits which tend to cluster. It supports up to >> > 'unsigned long' worth of bits. >> >> > lib/xbitmap.c| 444 >> > +++ >> >> Please, split tests to a separate module. > > Hah, I just did this two days ago! I didn't publish it yet, but I also made > it compile both in userspace and as a kernel module. > > It's the top two commits here: > > http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray-2018-02-12 > Thanks! > Note this is a complete rewrite compared to the version presented here; it > sits on top of the XArray and no longer has a preload interface. It has a > superset of the IDA functionality. Noted. Now, the question about test case. Why do you heavily use BUG_ON? Isn't resulting statistics enough? See how other lib/test_* modules do. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v21 1/5] xbitmap: Introduce xbitmap
On Tue, Jan 9, 2018 at 1:10 PM, Wei Wang <wei.w.w...@intel.com> wrote: > From: Matthew Wilcox <mawil...@microsoft.com> > > The eXtensible Bitmap is a sparse bitmap representation which is > efficient for set bits which tend to cluster. It supports up to > 'unsigned long' worth of bits. > lib/xbitmap.c| 444 > +++ Please, split tests to a separate module. -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 4/6] x86: Consolidate PCI_MMCONFIG configs
On Mon, Jan 22, 2018 at 8:12 AM, Jan Kiszka <jan.kis...@siemens.com> wrote: > From: Jan Kiszka <jan.kis...@siemens.com> > > Not sure if those two worked by design or just by chance so far. In any > case, it's at least cleaner and clearer to express this in a single > config statement. Congrats! You found by the way a bug in commit e279b6c1d329e50b766bce96aacc197eae8a053b Author: Sam Ravnborg <s...@ravnborg.org> Date: Tue Nov 6 20:41:05 2007 +0100 x86: start unification of arch/x86/Kconfig.* ...and proper fix seems to split PCI stuff to common + X86_32 only + X86_64 only -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1] virtio_blk: Use sysfs_match_string() helper
On Mon, 2017-07-03 at 19:37 +0300, Michael S. Tsirkin wrote: > On Mon, Jul 03, 2017 at 03:05:30PM +0300, Andy Shevchenko wrote: > > On Fri, 2017-06-09 at 15:07 +0300, Andy Shevchenko wrote: > > > Use sysfs_match_string() helper instead of open coded variant. > > > > Did I miss maintainer? > > > You didn't, I'll merge this in the next PULL. I didn't see this in v4.13-rc1. Shall I resend? -- Andy Shevchenko <andriy.shevche...@linux.intel.com> Intel Finland Oy ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1] virtio_blk: Use sysfs_match_string() helper
On Fri, 2017-06-09 at 15:07 +0300, Andy Shevchenko wrote: > Use sysfs_match_string() helper instead of open coded variant. Did I miss maintainer? > > Cc: "Michael S. Tsirkin" <m...@redhat.com> > Cc: Jason Wang <jasow...@redhat.com> > Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com> > --- > drivers/block/virtio_blk.c | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 553cc4c542b4..0e707b8cce9d 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -541,12 +541,9 @@ virtblk_cache_type_store(struct device *dev, > struct device_attribute *attr, > int i; > > BUG_ON(!virtio_has_feature(vblk->vdev, > VIRTIO_BLK_F_CONFIG_WCE)); > - for (i = ARRAY_SIZE(virtblk_cache_types); --i >= 0; ) > - if (sysfs_streq(buf, virtblk_cache_types[i])) > - break; > - > + i = sysfs_match_string(virtblk_cache_types, buf); > if (i < 0) > - return -EINVAL; > + return i; > > virtio_cwrite8(vdev, offsetof(struct virtio_blk_config, wce), > i); > virtblk_update_cache_mode(vdev); -- Andy Shevchenko <andriy.shevche...@linux.intel.com> Intel Finland Oy ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v1] virtio_blk: Use sysfs_match_string() helper
Use sysfs_match_string() helper instead of open coded variant. Cc: "Michael S. Tsirkin" <m...@redhat.com> Cc: Jason Wang <jasow...@redhat.com> Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com> --- drivers/block/virtio_blk.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 553cc4c542b4..0e707b8cce9d 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -541,12 +541,9 @@ virtblk_cache_type_store(struct device *dev, struct device_attribute *attr, int i; BUG_ON(!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_CONFIG_WCE)); - for (i = ARRAY_SIZE(virtblk_cache_types); --i >= 0; ) - if (sysfs_streq(buf, virtblk_cache_types[i])) - break; - + i = sysfs_match_string(virtblk_cache_types, buf); if (i < 0) - return -EINVAL; + return i; virtio_cwrite8(vdev, offsetof(struct virtio_blk_config, wce), i); virtblk_update_cache_mode(vdev); -- 2.11.0 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] vhost: replace % with & on data path
On Fri, Dec 4, 2015 at 10:19 PM, Venkatesh Srinivas <venkate...@google.com> wrote: > On Mon, Nov 30, 2015 at 11:15:23AM +0200, Michael S. Tsirkin wrote: >> We know vring num is a power of 2, so use & >> to mask the high bits. >> >> Signed-off-by: Michael S. Tsirkin <m...@redhat.com> >> --- > > The generated code switches from DIV -> masking, source is clearer as well. First impression was why, now it seems that compiler can't predict this for variables. For constants it would be optimized to the same, I suppose. > > Tested-by: Venkatesh Srinivas <venkate...@google.com> > > -- vs; > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- With Best Regards, Andy Shevchenko ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
