On Wed, Feb 01, 2023 at 06:32:29PM +0000, Michael Kelley (LINUX) wrote:
> From: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> Sent: Tuesday, January 31, 2023 10:10 AM
> >
> > Use more generic platform device instead of acpi device. Also rename the
> > function vmbus_acpi_remove to more generic name vmbus_mmio_remove.
> >
> > Signed-off-by: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/hv/vmbus_drv.c | 78 +++++++++++++++++++++++++-----------------
> > 1 file changed, 46 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index d24dd65b33d4..49030e756b9f 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -12,6 +12,7 @@
> > #include <linux/init.h>
> > #include <linux/module.h>
> > #include <linux/device.h>
> > +#include <linux/platform_device.h>
> > #include <linux/interrupt.h>
> > #include <linux/sysctl.h>
> > #include <linux/slab.h>
> > @@ -44,7 +45,7 @@ struct vmbus_dynid {
> > struct hv_vmbus_device_id id;
> > };
> >
> > -static struct acpi_device *hv_acpi_dev;
> > +static struct platform_device *hv_dev;
> >
> > static int hyperv_cpuhp_online;
> >
> > @@ -143,7 +144,7 @@ static DEFINE_MUTEX(hyperv_mmio_lock);
> >
> > static int vmbus_exists(void)
> > {
> > - if (hv_acpi_dev == NULL)
> > + if (hv_dev == NULL)
> > return -ENODEV;
> >
> > return 0;
> > @@ -932,7 +933,7 @@ static int vmbus_dma_configure(struct device *child_device)
> > * On x86/x64 coherence is assumed and these calls have no effect.
> > */
> > hv_setup_dma_ops(child_device,
> > - device_get_dma_attr(&hv_acpi_dev->dev) == DEV_DMA_COHERENT);
> > + device_get_dma_attr(&hv_dev->dev) == DEV_DMA_COHERENT);
> > return 0;
> > }
> >
> > @@ -2090,7 +2091,7 @@ int vmbus_device_register(struct hv_device
> > *child_device_obj)
> > &child_device_obj->channel->offermsg.offer.if_instance);
> >
> > child_device_obj->device.bus = &hv_bus;
> > - child_device_obj->device.parent = &hv_acpi_dev->dev;
> > + child_device_obj->device.parent = &hv_dev->dev;
> > child_device_obj->device.release = vmbus_device_release;
> >
> > child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> > @@ -2262,7 +2263,7 @@ static acpi_status vmbus_walk_resources(struct
> > acpi_resource *res, void *ctx)
> > return AE_OK;
> > }
> >
> > -static void vmbus_acpi_remove(struct acpi_device *device)
> > +static void vmbus_mmio_remove(void)
> > {
> > struct resource *cur_res;
> > struct resource *next_res;
> > @@ -2441,13 +2442,15 @@ void vmbus_free_mmio(resource_size_t start,
> > resource_size_t size)
> > }
> > EXPORT_SYMBOL_GPL(vmbus_free_mmio);
> >
> > -static int vmbus_acpi_add(struct acpi_device *device)
> > +static int vmbus_acpi_add(struct platform_device *pdev)
> > {
> > acpi_status result;
> > int ret_val = -ENODEV;
> > - struct acpi_device *ancestor;
> > + struct platform_device *ancestor;
> > + struct acpi_device *adev = to_acpi_device(&pdev->dev);
>
> This doesn't work. The argument to vmbus_acpi_add() is a struct
> platform_device, which has a struct device embedded in it (not a
> pointer). to_acpi_device() takes a struct device as an argument,
> assuming that the struct device is embedded in a struct
> acpi_device, which is not the case here. The resulting local
> variable adev is actually pointing to some (perhaps negative)
> offset within the struct platform_device, and uses of adev are
> getting unknown random data from within (or before) the
> struct platform_device.
Thanks for review.
If I understand correctly you are saying to change the argument of
to_acpi_device from pointer to 'struct device'. I tried changing this
statement to:
"struct acpi_device *adev = to_acpi_device(pdev->dev);"
However on compiling this code I see this error:
./include/linux/container_of.h:19:28: error: invalid type argument of unary ‘*’ (have ‘struct device’)
Please let me know if I missunderstood your suggestion.
I would like to mention that this code has been tested and verified.
The purpose of this change was to assign the device node to the acpi
firmware node, as seen in the line
"adev->fwnode.dev = &pdev->dev;".
This was necessary because without it, the mlx driver would crash as
it searches for the device in the acpi firmware node, which would
otherwise be NULL. I am confident that the addresses are correctly
assigned, as any issues with the assignments would have caused a panic.
Regards,
Saurabh
>
> >
> > - hv_acpi_dev = device;
> > + hv_dev = pdev;
> > + adev->fwnode.dev = &pdev->dev;
> >
> > /*
> > * Older versions of Hyper-V for ARM64 fail to include the _CCA
> > @@ -2456,15 +2459,16 @@ static int vmbus_acpi_add(struct acpi_device *device)
> > * up the ACPI device to behave as if _CCA is present and indicates
> > * hardware coherence.
> > */
> > - ACPI_COMPANION_SET(&device->dev, device);
> > + ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pdev->dev));
>
> This statement seems tautological. If ACPI_COMPANION(&pdev->dev)
> returns a valid result, why would the ACPI companion for &pdev->dev
> need to be set? The original code was setting the ACPI companion for the
> embedded struct device to be the struct acpi_device. I forget why this
> wasn't already done for the VMBus device when it was originally parsed
> from the ACPI DSDT ...
>
> > if (IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) &&
> > - device_get_dma_attr(&device->dev) == DEV_DMA_NOT_SUPPORTED) {
> > + device_get_dma_attr(&pdev->dev) == DEV_DMA_NOT_SUPPORTED) {
> > + struct acpi_device *adev_node = ACPI_COMPANION(&pdev->dev);
>
> If earlier code in this function can get a correct pointer to the struct acpi_device,
> then this statement shouldn't be necessary. You already have it.
>
> > pr_info("No ACPI _CCA found; assuming coherent device I/O\n");
> > - device->flags.cca_seen = true;
> > - device->flags.coherent_dma = true;
> > + adev_node->flags.cca_seen = true;
> > + adev_node->flags.coherent_dma = true;
> > }
> >
> > - result = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> > + result = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), METHOD_NAME__CRS,
>
> Again, if you have a correct pointer to the struct acpi_device, then adev->handle
> (like the original code) should be simpler than looking it up again with ACPI_HANDLE().
>
> > vmbus_walk_resources, NULL);
> >
> > if (ACPI_FAILURE(result))
> > @@ -2473,9 +2477,9 @@ static int vmbus_acpi_add(struct acpi_device *device)
> > * Some ancestor of the vmbus acpi device (Gen1 or Gen2
> > * firmware) is the VMOD that has the mmio ranges. Get that.
> > */
> > - for (ancestor = acpi_dev_parent(device); ancestor;
> > - ancestor = acpi_dev_parent(ancestor)) {
> > - result = acpi_walk_resources(ancestor->handle, METHOD_NAME__CRS,
> > + for (ancestor = to_platform_device(pdev->dev.parent); ancestor;
> > + ancestor = to_platform_device(ancestor->dev.parent)) {
> > + result = acpi_walk_resources(ACPI_HANDLE(&ancestor->dev), METHOD_NAME__CRS,
>
> Similarly, if you get a correct pointer to the struct acpi_device, does the above
> code need any changes? I'm hoping not.
>
> > vmbus_walk_resources, NULL);
> >
> > if (ACPI_FAILURE(result))
> > @@ -2489,10 +2493,21 @@ static int vmbus_acpi_add(struct acpi_device *device)
> >
> > acpi_walk_err:
> > if (ret_val)
> > - vmbus_acpi_remove(device);
> > + vmbus_mmio_remove();
> > return ret_val;
> > }
> >
> > +static int vmbus_platform_driver_probe(struct platform_device *pdev)
> > +{
> > + return vmbus_acpi_add(pdev);
> > +}
> > +
> > +static int vmbus_platform_driver_remove(struct platform_device *pdev)
> > +{
> > + vmbus_mmio_remove();
> > + return 0;
> > +}
> > +
> > #ifdef CONFIG_PM_SLEEP
> > static int vmbus_bus_suspend(struct device *dev)
> > {
> > @@ -2658,15 +2673,15 @@ static const struct dev_pm_ops vmbus_bus_pm = {
> > .restore_noirq = vmbus_bus_resume
> > };
> >
> > -static struct acpi_driver vmbus_acpi_driver = {
> > - .name = "vmbus",
> > - .ids = vmbus_acpi_device_ids,
> > - .ops = {
> > - .add = vmbus_acpi_add,
> > - .remove = vmbus_acpi_remove,
> > - },
> > - .drv.pm = &vmbus_bus_pm,
> > - .drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
> > +static struct platform_driver vmbus_platform_driver = {
> > + .probe = vmbus_platform_driver_probe,
> > + .remove = vmbus_platform_driver_remove,
> > + .driver = {
> > + .name = "vmbus",
> > + .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
> > + .pm = &vmbus_bus_pm,
> > + .probe_type = PROBE_FORCE_SYNCHRONOUS,
> > + }
> > };
> >
> > static void hv_kexec_handler(void)
> > @@ -2750,12 +2765,11 @@ static int __init hv_acpi_init(void)
> > /*
> > * Get ACPI resources first.
> > */
> > - ret = acpi_bus_register_driver(&vmbus_acpi_driver);
> > -
> > + ret = platform_driver_register(&vmbus_platform_driver);
> > if (ret)
> > return ret;
> >
> > - if (!hv_acpi_dev) {
> > + if (!hv_dev) {
> > ret = -ENODEV;
> > goto cleanup;
> > }
> > @@ -2785,8 +2799,8 @@ static int __init hv_acpi_init(void)
> > return 0;
> >
> > cleanup:
> > - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > - hv_acpi_dev = NULL;
> > + platform_driver_unregister(&vmbus_platform_driver);
> > + hv_dev = NULL;
> > return ret;
> > }
> >
> > @@ -2839,7 +2853,7 @@ static void __exit vmbus_exit(void)
> >
> > cpuhp_remove_state(hyperv_cpuhp_online);
> > hv_synic_free();
> > - acpi_bus_unregister_driver(&vmbus_acpi_driver);
> > + platform_driver_unregister(&vmbus_platform_driver);
> > }
> >
> >
> > --
> > 2.25.1
