linux-kernel - Re: [PATCH v8 1/1] misc: mrvl-cn10k-dpi: add Octeon CN10K DPI administrative driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Tue, 18 Jun 2024 16:19:25 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Vamsi Attunuru" <vattunuru@...vell.com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc: "Jerin Jacob" <jerinj@...vell.com>,
 "Srujana Challa" <schalla@...vell.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 1/1] misc: mrvl-cn10k-dpi: add Octeon CN10K DPI administrative
 driver

On Tue, Jun 18, 2024, at 15:09, Vamsi Attunuru wrote:
>Changes V7 -> V8:
> - Used bit shift operations to access mbox msg fields
> - Removed bitfields in mailbox msg structure

Thanks for the changes, looks good to me. Two more things:

> +static void dpi_poll_pfvf_mbox(struct dpipf *dpi)
> +{
> +	u64 reg;
> +	u32 vf;
> +
> +	reg = dpi_reg_read(dpi, DPI_MBOX_VF_PF_INT);
> +	if (reg) {
> +		for (vf = 0; vf < pci_num_vf(dpi->pdev); vf++) {
> +			if (reg & BIT_ULL(vf))
> +				schedule_work(&dpi->mbox[vf]->work);
> +		}
> +		dpi_reg_write(dpi, DPI_MBOX_VF_PF_INT, reg);
> +	}
> +}
> +
> +static irqreturn_t dpi_mbox_intr_handler(int irq, void *data)
> +{
> +	struct dpipf *dpi = data;
> +
> +	dpi_poll_pfvf_mbox(dpi);
> +
> +	return IRQ_HANDLED;
> +}

[minor cleanup]
The second function doesn't do much here, so you could
just merge them into one. Or to simplify it further, you
could just use request_threaded_irq() and avoid the separate
workqueue as well.

> +struct dpi_mps_mrrs_cfg {
> +	__u16 max_read_req_sz; /* Max read request size */
> +	__u16 max_payload_sz;  /* Max payload size */
> +	__u8 port; /* Ebus port */
> +};
> +
> +struct dpi_engine_cfg {
> +	__u64 fifo_mask; /* FIFO size mask in KBytes */
> +	__u16 molr[DPI_MAX_ENGINES]; /* Max outstanding load requests */
> +	__u8 update_molr; /* '1' to update engine MOLR */
> +};

Both of these structures have architecture specific padding
at the end because the members don't add up to a multiple
of words. Please add explicit reserved fields to a multiple of
8 bytes for each one, or make the fields longer.

Documentation/driver-api/ioctl.rst has more details on this.

     Arnd