| 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: <9cc6bdf8-ba4c-4561-962a-74ceb09b72a8@quicinc.com>
Date: Sat, 3 May 2025 16:47:52 +0530
From: Viken Dadhaniya <quic_vdadhani@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, <andi.shyti@...nel.org>,
<robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<gregkh@...uxfoundation.org>, <jirislaby@...nel.org>,
<broonie@...nel.or>, <andersson@...nel.org>, <konradybcio@...nel.org>,
<johan+linaro@...nel.org>, <dianders@...omium.org>,
<agross@...nel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-serial@...r.kernel.org>,
<linux-spi@...r.kernel.org>
CC: <quic_msavaliy@...cinc.com>, <quic_anupkulk@...cinc.com>
Subject: Re: [PATCH v3 6/9] soc: qcom: geni-se: Add support to load QUP SE
Firmware via Linux subsystem
On 3/8/2025 11:36 PM, Konrad Dybcio wrote:
> On 3.03.2025 1:43 PM, Viken Dadhaniya wrote:
>> Load the firmware to QUP SE based on the 'firmware-name' property specified
>> in devicetree. Populate Serial engine and base address details in the probe
>> function of the protocol driver and pass to firmware load routine.
>>
>> Skip the firmware loading if the firmware is already loaded in Serial
>> Engine's firmware memory area.
>>
>> Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@...cinc.com>
>> ---
>
> [...]
>
>> +static bool elf_phdr_valid(const struct elf32_phdr *phdr)
>> +{
>> + if (phdr->p_type != PT_LOAD || !phdr->p_memsz)
>> + return false;
>> +
>> + if (MI_PBT_PAGE_MODE_VALUE(phdr->p_flags) == MI_PBT_NON_PAGED_SEGMENT &&
>> + MI_PBT_SEGMENT_TYPE_VALUE(phdr->p_flags) != MI_PBT_HASH_SEGMENT &&
>> + MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) != MI_PBT_NOTUSED_SEGMENT &&
>> + MI_PBT_ACCESS_TYPE_VALUE(phdr->p_flags) != MI_PBT_SHARED_SEGMENT)
>> + return true;
>> +
>> + return false;
>
> return (contents of the if condition)
>
>> +}
>> +
>> +/**
>> + * valid_seg_size() - Validate the segment size.
>> + * @pelfseg: Pointer to the ELF header.
>> + * @p_filesz: Pointer to the file size.
>> + *
>> + * Validate the ELF segment size by comparing the file size.
>> + *
>> + * Return: true if the segment is valid, false if the segment is invalid.
>> + */
>> +static bool valid_seg_size(struct elf_se_hdr *pelfseg, Elf32_Word p_filesz)
>> +{
>> + if (p_filesz >= pelfseg->fw_offset + pelfseg->fw_size_in_items * sizeof(u32) &&
>> + p_filesz >= pelfseg->cfg_idx_offset + pelfseg->cfg_size_in_items * sizeof(u8) &&
>> + p_filesz >= pelfseg->cfg_val_offset + pelfseg->cfg_size_in_items * sizeof(u32))
>> + return true;
>> + return false;
>> +}
>
> same here
>
> [...]
>
>> +static int geni_configure_xfer_mode(struct qup_se_rsc *rsc)
>> +{
>> + /* Configure SE FIFO, DMA or GSI mode. */
>> + switch (rsc->mode) {
>> + case GENI_GPI_DMA:
>> + setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> + GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> + writel_relaxed(0x0, rsc->se->base + SE_IRQ_EN);
>> + writel_relaxed(SE_GSI_EVENT_EN_BMSK, rsc->se->base + SE_GSI_EVENT_EN);
>> + break;
>> +
>> + case GENI_SE_FIFO:
>> + clrbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> + GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>> + writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> + writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>> + break;
>> +
>> + case GENI_SE_DMA:
>> + setbits32(rsc->se->base + QUPV3_SE_GENI_DMA_MODE_EN,
>> + GENI_DMA_MODE_EN_GENI_DMA_MODE_EN_BMSK);
>
> This write is common across all 3 modes
In FIFO mode, the operation is to clear the bit, while in DMA mode, the
operation is to set the bit.
>
>> + writel_relaxed(SE_IRQ_EN_RMSK, rsc->se->base + SE_IRQ_EN);
>> + writel_relaxed(0x0, rsc->se->base + SE_GSI_EVENT_EN);
>
> These two writes are common across !GPI_DMA
We have different operations in all three modes, so it's not possible to
combine any of them.
>
>> + break;
>> +
>> + default:
>> + dev_err(rsc->se->dev, "invalid se mode: %d\n", rsc->mode);
>> + return -EINVAL;
>
> I wouldn't expect this to ever fail..
Yes, that's correct. But including a default case helps handle
unexpected or invalid input gracefully.
Please let me know if you would like me to remove it.
>
>> + }
>> + return 0;
>> +}
>> +
>> +/**
>> + * geni_enable_interrupts() Enable interrupts.
>> + * @rsc: Pointer to a structure representing SE-related resources.
>> + *
>> + * Enable the required interrupts during the firmware load process.
>> + *
>> + * Return: None.
>> + */
>> +static void geni_enable_interrupts(struct qup_se_rsc *rsc)
>> +{
>> + u32 reg_value;
>> +
>> + /* Enable required interrupts. */
>> + writel_relaxed(M_COMMON_GENI_M_IRQ_EN, rsc->se->base + GENI_M_IRQ_ENABLE);
>> +
>> + reg_value = S_CMD_OVERRUN_EN | S_ILLEGAL_CMD_EN |
>> + S_CMD_CANCEL_EN | S_CMD_ABORT_EN |
>> + S_GP_IRQ_0_EN | S_GP_IRQ_1_EN |
>> + S_GP_IRQ_2_EN | S_GP_IRQ_3_EN |
>> + S_RX_FIFO_WR_ERR_EN | S_RX_FIFO_RD_ERR_EN;
>
> The S-es should be aligned, similarly for other additions in this patch
Sure, updated in v4.
>
> [...]
>
>> + /* Flash firmware revision register. */
>> + reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> + (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>
> Use FIELD_PREP and GENMASK to denote bitfields
Sure, updated in v4.
>
>> + writel_relaxed(reg_value, rsc->se->base + SE_GENI_FW_REVISION);
>> +
>> + reg_value = (hdr->serial_protocol << FW_REV_PROTOCOL_SHFT) |
>> + (hdr->fw_version & 0xFF << FW_REV_VERSION_SHFT);
>> +
>> + writel_relaxed(reg_value, rsc->se->base + SE_S_FW_REVISION);
>> +}
>
> Konrad
Powered by blists - more mailing lists