Re: [PATCH v6 2/4] drivers/acpi: Introduce Platform Firmware Runtime Update device driver

[Chen Yu <yu.c.chen@intel.com>]

On Mon, Oct 25, 2021 at 08:47:42AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 25, 2021 at 02:25:16PM +0800, Chen Yu wrote:
[snip...]
> > +
> > +static int acpi_pfru_probe(struct platform_device *pdev)
> > +{
> > +	struct pfru_device *pfru_dev;
> > +	acpi_handle handle;
> > +	static int pfru_idx;
> 
> Why not use an ida/idr structure for this?  You never decrement this
> when the device is removed?
>
Will fix it and use ida_alloc() for this in next version. 
> > +	int ret;
> > +
> > +	pfru_dev = devm_kzalloc(&pdev->dev, sizeof(*pfru_dev), GFP_KERNEL);
> > +	if (!pfru_dev)
> > +		return -ENOMEM;
> > +
> > +	ret = guid_parse(PFRU_UUID, &pfru_dev->uuid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = guid_parse(PFRU_CODE_INJ_UUID, &pfru_dev->code_uuid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = guid_parse(PFRU_DRV_UPDATE_UUID, &pfru_dev->drv_uuid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* default rev id is 1 */
> > +	pfru_dev->rev_id = 1;
> > +	pfru_dev->dev = &pdev->dev;
> > +	handle = ACPI_HANDLE(pfru_dev->dev);
> > +	if (!acpi_has_method(handle, "_DSM")) {
> > +		dev_dbg(&pdev->dev, "Missing _DSM\n");
> > +		return -ENODEV;
> > +	}
> 
> Why not make this check first, before you allocate or parse anything?
>
Will fix it in next version. 
> > +
> > +	pfru_dev->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	pfru_dev->miscdev.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +						"pfru%d", pfru_idx);
> > +	if (!pfru_dev->miscdev.name)
> > +		return -ENOMEM;
> > +
> > +	pfru_dev->miscdev.nodename = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > +						    "acpi_pfru%d", pfru_idx);
> > +	if (!pfru_dev->miscdev.nodename)
> > +		return -ENOMEM;
> > +
> > +	pfru_idx++;
> > +	pfru_dev->miscdev.fops = &acpi_pfru_fops;
> > +
> > +	ret = misc_register(&pfru_dev->miscdev);
> > +	if (ret)
> > +		return ret;
> 
> You forgot to set the parent of the misc device here, right?  :(
> 
>
Ah, yes, will fix it in next version. 
> > +
> > +	platform_set_drvdata(pdev, pfru_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct acpi_device_id acpi_pfru_ids[] = {
> > +	{"INTC1080", 0},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, acpi_pfru_ids);
> > +
> > +static struct platform_driver acpi_pfru_driver = {
> > +	.driver = {
> > +		.name = "pfru_update",
> > +		.acpi_match_table = acpi_pfru_ids,
> > +	},
> > +	.probe = acpi_pfru_probe,
> > +	.remove = acpi_pfru_remove,
> > +};
> > +
> > +static int __init pfru_init(void)
> > +{
> > +	return platform_driver_register(&acpi_pfru_driver);
> > +}
> > +
> > +static void __exit pfru_exit(void)
> > +{
> > +	platform_driver_unregister(&acpi_pfru_driver);
> > +}
> > +
> > +module_init(pfru_init);
> > +module_exit(pfru_exit);
> 
> module_platform_driver()?
>
Currently there are two platform drivers in this file, one is this
platform driver, another one will be introduced in the subsequent
patch for telemetry. Since the two platform drivers are treated
as a whole, they are put into one file. Should I split them
into two files?

thanks,
Chenyu 
> thanks,
> 
> greg k-h