| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3991ab78-d1a2-4cae-bea5-fb4dfa58aba3@wanadoo.fr>
Date: Thu, 16 Jan 2025 19:03:13 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: devnull+fnkl.kernel.gmail.com@...nel.org
Cc: alyssa@...enzweig.io, asahi@...ts.linux.dev, conor+dt@...nel.org,
devicetree@...r.kernel.org, dmitry.torokhov@...il.com,
fnkl.kernel@...il.com, j@...nau.net, krzk+dt@...nel.org,
linux-arm-kernel@...ts.infradead.org, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, marcan@...can.st, neal@...pa.dev,
robh@...nel.org, rydberg@...math.org, sven@...npeter.dev
Subject: Re: [PATCH v4 2/4] input: apple_z2: Add a driver for Apple Z2
touchscreens
Le 15/01/2025 à 23:46, Sasha Finkelstein via B4 Relay a écrit :
> From: Sasha Finkelstein <fnkl.kernel-Re5JQEeQqe8AvxtiuMwx3w@...lic.gmane.org>
>
> Adds a driver for Apple touchscreens using the Z2 protocol.
>
...
> +static int apple_z2_upload_firmware(struct apple_z2 *z2)
> +{
> + const struct apple_z2_fw_hdr *fw_hdr;
> + size_t fw_idx = sizeof(struct apple_z2_fw_hdr);
> + int error;
> + u32 load_cmd;
> + u32 size;
> + u32 address;
> + char *data;
> + bool init;
> + size_t cal_size;
> +
> + const struct firmware *fw __free(firmware) = NULL;
> + error = request_firmware(&fw, z2->fw_name, &z2->spidev->dev);
> + if (error) {
> + dev_err(&z2->spidev->dev, "unable to load firmware");
> + return error;
> + }
> +
> + fw_hdr = (const struct apple_z2_fw_hdr *)fw->data;
> + if (le32_to_cpu(fw_hdr->magic) != APPLE_Z2_FW_MAGIC || le32_to_cpu(fw_hdr->version) != 1) {
> + dev_err(&z2->spidev->dev, "invalid firmware header");
> + return -EINVAL;
> + }
> +
> + /*
> + * This will interrupt the upload half-way if the file is malformed
> + * As the device has no non-volatile storage to corrupt, and gets reset
> + * on boot anyway, this is fine.
> + */
> + while (fw_idx < fw->size) {
> + if (fw->size - fw_idx < 8) {
> + dev_err(&z2->spidev->dev, "firmware malformed");
> + return -EINVAL;
> + }
> +
> + load_cmd = le32_to_cpu(*(__le32 *)(fw->data + fw_idx));
> + fw_idx += sizeof(u32);
> + if (load_cmd == LOAD_COMMAND_INIT_PAYLOAD || load_cmd == LOAD_COMMAND_SEND_BLOB) {
> + size = le32_to_cpu(*(__le32 *)(fw->data + fw_idx));
> + fw_idx += sizeof(u32);
> + if (fw->size - fw_idx < size) {
> + dev_err(&z2->spidev->dev, "firmware malformed");
> + return -EINVAL;
> + }
> + init = load_cmd == LOAD_COMMAND_INIT_PAYLOAD;
> + error = apple_z2_send_firmware_blob(z2, fw->data + fw_idx,
> + size, init);
> + if (error)
> + return error;
> + fw_idx += size;
> + } else if (load_cmd == LOAD_COMMAND_SEND_CALIBRATION) {
> + address = le32_to_cpu(*(u32 *)(fw->data + fw_idx));
> + fw_idx += sizeof(u32);
> + cal_size = device_property_count_u8(&z2->spidev->dev, CAL_PROP_NAME);
> + if (cal_size != 0) {
> + size = cal_size + sizeof(struct apple_z2_hbpp_blob_hdr) + 4;
> + data = kzalloc(size, GFP_KERNEL);
> + error = apple_z2_build_cal_blob(z2, address, cal_size, data);
> + if (!error)
> + error = apple_z2_send_firmware_blob(z2, data, size, 16);
> + kfree(data);
> + if (error)
> + return error;
> + }
> + } else {
> + dev_err(&z2->spidev->dev, "firmware malformed");
Missing \n.
> + return -EINVAL;
> + }
> + if (fw_idx % 4 != 0)
> + fw_idx += 4 - (fw_idx % 4);
> + }
> +
> +
> + z2->booted = true;
> + apple_z2_read_packet(z2);
> + return 0;
> +}
...
> +static int apple_z2_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct apple_z2 *z2;
> + int error;
> +
> + z2 = devm_kzalloc(dev, sizeof(*z2), GFP_KERNEL);
> + if (!z2)
> + return -ENOMEM;
> +
> + z2->tx_buf = devm_kzalloc(dev, sizeof(struct apple_z2_read_interrupt_cmd), GFP_KERNEL);
> + z2->rx_buf = devm_kzalloc(dev, 4096, GFP_KERNEL);
This will allocate 8192 bytes because of the way the allocator works.
It needs around 40 bytes for the devm stuff + 4096 requested. So
rounding rules will allocate 8192 bytes.
So either you could allocate "for free" much more space, or you could
allocate (and document...)
z2->rx_buf = devm_kzalloc(dev, 4096 - sizeof(struct devres), GFP_KERNEL);
or have an explicit devm_add_action_or_reset() that would require less
memory, but would add some LoC.
See
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/base/devres.c#L97
> + if (!z2->tx_buf || !z2->rx_buf)
> + return -ENOMEM;
> +
> + z2->spidev = spi;
> + init_completion(&z2->boot_irq);
> + spi_set_drvdata(spi, z2);
> +
> + /* Reset the device on boot */
> + z2->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(z2->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(z2->reset_gpio), "unable to get reset");
> +
> + error = devm_request_threaded_irq(dev, z2->spidev->irq, NULL,
> + apple_z2_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN,
> + "apple-z2-irq", spi);
> + if (error)
> + return dev_err_probe(dev, z2->spidev->irq, "unable to request irq");
Missing \n.
s/z2->spidev->irq/error/ ?
or maybe:
return dev_err_probe(dev, error, "unable to request irq %d\n",
z2->spidev->irq);
> +
> + error = device_property_read_string(dev, "firmware-name", &z2->fw_name);
> + if (error)
> + return dev_err_probe(dev, error, "unable to get firmware name");
Missing \n.
> +
> + z2->input_dev = devm_input_allocate_device(dev);
> + if (!z2->input_dev)
> + return -ENOMEM;
> + z2->input_dev->name = (char *)spi_get_device_id(spi)->driver_data;
> + z2->input_dev->phys = "apple_z2";
> + z2->input_dev->id.bustype = BUS_SPI;
> +
> + /* Allocate the axes before setting from DT */
> + input_set_abs_params(z2->input_dev, ABS_MT_POSITION_X, 0, 0, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_POSITION_Y, 0, 0, 0, 0);
> + touchscreen_parse_properties(z2->input_dev, true, &z2->props);
> + input_abs_set_res(z2->input_dev, ABS_MT_POSITION_X, 100);
> + input_abs_set_res(z2->input_dev, ABS_MT_POSITION_Y, 100);
> + input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MAJOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_WIDTH_MINOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MAJOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_TOUCH_MINOR, 0, 65535, 0, 0);
> + input_set_abs_params(z2->input_dev, ABS_MT_ORIENTATION, -32768, 32767, 0, 0);
> +
> + input_set_drvdata(z2->input_dev, z2);
Is it needed? (there is no input_get_drvdata())
> +
> + error = input_mt_init_slots(z2->input_dev, 256, INPUT_MT_DIRECT);
> + if (error)
> + return dev_err_probe(dev, error, "unable to initialize multitouch slots");
Missing \n.
> +
> + error = input_register_device(z2->input_dev);
> + if (error)
> + return dev_err_probe(dev, error, "unable to register input device");
Missing \n.
> +
> + /* Wait for device reset to finish */
> + usleep_range(5000, 10000);
> + error = apple_z2_boot(z2);
> + if (error)
> + return error;
> + return 0;
Nitpick: These 4 lines could be just:
return apple_z2_boot(z2);
> +}
...
> +static DEFINE_SIMPLE_DEV_PM_OPS(apple_z2_pm, apple_z2_suspend, apple_z2_resume);
> +
> +static const struct of_device_id apple_z2_of_match[] = {
> + { .compatible = "apple,j293-touchbar" },
> + { .compatible = "apple,j493-touchbar" },
> + {},
Nitpick: Ending comma is not needed after a terminator.
> +};
> +MODULE_DEVICE_TABLE(of, apple_z2_of_match);
> +
> +static struct spi_device_id apple_z2_of_id[] = {
> + { .name = "j293-touchbar", .driver_data = (kernel_ulong_t)"MacBookPro17,1 Touch Bar" },
> + { .name = "j493-touchbar", .driver_data = (kernel_ulong_t)"Mac14,7 Touch Bar" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, apple_z2_of_id);
...
CJ
Powered by blists - more mailing lists