linux-kernel - Re: [PATCH v3 2/2] mailbox: apple: Add driver for Apple mailboxes

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 17 Oct 2021 08:02:39 -0400
From:   Alyssa Rosenzweig <alyssa@...enzweig.io>
To:     Sven Peter <sven@...npeter.dev>
Cc:     Jassi Brar <jassisinghbrar@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Kettenis <mark.kettenis@...all.nl>,
        Hector Martin <marcan@...can.st>,
        Mohamed Mediouni <mohamed.mediouni@...amail.com>,
        Stan Skowronek <stan@...ellium.com>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] mailbox: apple: Add driver for Apple mailboxes

> Apple SoCs such as the M1 come with various co-processors. Mailboxes
> are used to communicate with those. This driver adds support for
> two variants of those mailboxes.
> 
> Reviewed-by: Alyssa Rosenzweig <alyssa@...enzweig.io>
> Signed-off-by: Sven Peter <sven@...npeter.dev>

In the future, Reviewed-by tags should be dropped after making major
changes to a patch.

> +		writel_relaxed(apple_mbox->hw->irq_bit_recv_not_empty |
> +				       apple_mbox->hw->irq_bit_send_empty,
> +			       apple_mbox->regs + apple_mbox->hw->irq_enable);

Nit: weird wrapping, much easier to read as:

+		writel_relaxed(apple_mbox->hw->irq_bit_recv_not_empty |
+			       apple_mbox->hw->irq_bit_send_empty,
+			       apple_mbox->regs + apple_mbox->hw->irq_enable);

> +static const struct apple_mbox_hw apple_mbox_asc_hw = {
> +	.control_full = APPLE_ASC_MBOX_CONTROL_FULL,
> +	.control_empty = APPLE_ASC_MBOX_CONTROL_EMPTY,
> +
> +	.a2i_control = APPLE_ASC_MBOX_A2I_CONTROL,
> +	.a2i_send0 = APPLE_ASC_MBOX_A2I_SEND0,
> +	.a2i_send1 = APPLE_ASC_MBOX_A2I_SEND1,
> +
> +	.i2a_control = APPLE_ASC_MBOX_I2A_CONTROL,
> +	.i2a_recv0 = APPLE_ASC_MBOX_I2A_RECV0,
> +	.i2a_recv1 = APPLE_ASC_MBOX_I2A_RECV1,
> +
> +	.has_irq_controls = false,
> +};

Nit: consider dropping the `has_irq_controls = false` assignment.
Clearly there are none, or you'd have to fill out the irq_* fields too.

> +static const struct of_device_id apple_mbox_of_match[] = {
> +	{ .compatible = "apple,t8103-asc-mailbox", .data = &apple_mbox_asc_hw },
> +	{ .compatible = "apple,t8103-m3-mailbox", .data = &apple_mbox_m3_hw },
> +	{}
> +};

No generic compatibles? I assume this driver hasn't changed much in
recent iPhones, and hopefully it won't change much in M1X...

> +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> +/*
> + * Apple mailbox message format
> + *
> + * Copyright (C) 2021 The Asahi Linux Contributors
> + */
> +
> +#ifndef _LINUX_APPLE_MAILBOX_H_
> +#define _LINUX_APPLE_MAILBOX_H_
> +
> +#include <linux/types.h>
> +
> +struct apple_mbox_msg {
> +	u64 msg0;
> +	u32 msg1;
> +};
> +
> +#endif

Given this file lacks the context of the driver, and the questions
raised in v2 review, it might be beneficial to add a quick comment to
apple_mbox_msg explaiing that no, really, this is a 96-bit message.