Hi,

Thanks for reviewing the patch

On Thu, 9 Dec 2021, Russell King (Oracle) wrote:

Hi,

On Thu, Dec 02, 2021 at 06:43:11PM -0800, Ilkka Koskinen wrote:

quoted

+static int __init agdi_init(void)
+{
+	int ret;
+	acpi_status status;
+	struct acpi_table_agdi *agdi_table;
+	struct agdi_data *pdata;
+	struct platform_device *pdev;
+
+	if (acpi_disabled)
+		return 0;
+
+	status = acpi_get_table(ACPI_SIG_AGDI, 0,
+				(struct acpi_table_header **) &agdi_table);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_ATOMIC);

Why does this need to be GFP_ATOMIC? Also, struct agdi_data is a single
int in size, why do you need to kzalloc() it?

There's no reason for that. It should obviously be GFP_KERNEL

quoted

+	if (!pdata) {
+		ret = -ENOMEM;
+		goto err_put_table;
+	}
+
+	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) {
+		pr_warn("Interrupt signaling is not supported");
+		ret = -ENODEV;
+		goto err_free_pdata;
+	}
+
+	pdata->sdei_event = agdi_table->sdei_event;
+
+	pdev = platform_device_register_data(NULL, "agdi", 0, pdata, sizeof(*pdata));

platform_device_register_data() uses kmemdup() internally with the
platform data, meaning it takes a copy of the platform data. There is
no need for the pdata allocation to persist past this point. Hence,
given that it is a single int in size, you may as well put
"struct agdi_data" on the stack.

I somehow missed kmemdup() part. I remove the allocation and move pdata to 
the stack instead.

Thanks,
Ilkka

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel