| 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:
<PSAPR06MB49498DB3E5D1F2E6261DAF4889FF2@PSAPR06MB4949.apcprd06.prod.outlook.com>
Date: Thu, 13 Feb 2025 08:57:34 +0000
From: Kevin Chen <kevin_chen@...eedtech.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, "joel@....id.au" <joel@....id.au>,
"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "elbadrym@...gle.com" <elbadrym@...gle.com>
Subject: RE: [PATCH v0 3/3] soc: aspeed: lpc-pcc: Add PCC controller support
> On 17/01/2025 10:52, Kevin Chen wrote:
> > Add LPC PCC controller driver to support POST code capture.
> >
> > Signed-off-by: Kevin Chen <kevin_chen@...eedtech.com>
> > ---
>
>
> This patch was never tested with your DTS and bindings. Please do not send
> untested code.
For the dts, I asked Google to upstream their dts using lpc_pcc node.
>
> > +
> > +/*
> > + * A2600-15 AP note
> > + *
> > + * SW workaround to prevent generating Non-Fatal-Error (NFE)
> > + * eSPI response when PCC is used for port I/O byte snooping
> > + * over eSPI.
> > + */
> > +static int aspeed_a2600_15(struct aspeed_pcc *pcc, struct device
> > +*dev) {
> > + struct device_node *np;
> > + u32 hicrb_en;
> > +
> > + /* abort if snoop is enabled */
> > + np = of_find_compatible_node(dev->parent->of_node, NULL,
> > +"aspeed,ast2600-lpc-snoop");
>
> No, don't sprinkle compatible to epxress relationship. Use proper phandles or
> parent-child relationship.
Agree, I will remove it.
>
> > + if (np) {
> > + if (of_device_is_available(np)) {
> > + dev_err(dev, "A2600-15 should be applied with snoop
> disabled\n");
> > + return -EPERM;
> > + }
> > + }
> > +
> > + /* abort if port is not 4-bytes continuous range */
> > + if (pcc->port_xbits != 0x3) {
> > + dev_err(dev, "A2600-15 should be applied on 4-bytes continuous I/O
> address range\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* set SNPWADR of snoop device */
> > + regmap_write(pcc->regmap, SNPWADR, pcc->port | ((pcc->port + 2) <<
> > +16));
> > +
> > + /* set HICRB[15:14]=11b to enable ACCEPT response for SNPWADR */
> > + hicrb_en = HICRB_ENSNP0D | HICRB_ENSNP1D;
> > + regmap_update_bits(pcc->regmap, HICRB, hicrb_en, hicrb_en);
> > +
> > + /* set HICR6[19] to extend SNPWADR to 2x range */
> > + regmap_update_bits(pcc->regmap, HICR6, HICR6_EN2BMODE,
> > +HICR6_EN2BMODE);
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_pcc_enable(struct aspeed_pcc *pcc, struct device
> > +*dev) {
> > + int rc;
> > +
> > + if (pcc->a2600_15) {
> > + rc = aspeed_a2600_15(pcc, dev);
> > + if (rc)
> > + return rc;
> > + }
> > +
> > + /* record mode */
> > + regmap_update_bits(pcc->regmap, PCCR0,
> > + PCCR0_MODE_SEL_MASK,
> > + pcc->rec_mode << PCCR0_MODE_SEL_SHIFT);
> > +
> > + /* port address */
> > + regmap_update_bits(pcc->regmap, PCCR1,
> > + PCCR1_BASE_ADDR_MASK,
> > + pcc->port << PCCR1_BASE_ADDR_SHIFT);
> > +
> > + /* port address high bits selection or parser control */
> > + regmap_update_bits(pcc->regmap, PCCR0,
> > + PCCR0_ADDR_SEL_MASK,
> > + pcc->port_hbits_select << PCCR0_ADDR_SEL_SHIFT);
> > +
> > + /* port address dont care bits */
> > + regmap_update_bits(pcc->regmap, PCCR1,
> > + PCCR1_DONT_CARE_BITS_MASK,
> > + pcc->port_xbits << PCCR1_DONT_CARE_BITS_SHIFT);
> > +
> > + /* set DMA ring buffer size and enable interrupts */
> > + if (pcc->dma_mode) {
> > + regmap_write(pcc->regmap, PCCR4, pcc->dma.addr & 0xffffffff);
> > + regmap_update_bits(pcc->regmap, PCCR5,
> PCCR5_DMA_ADDRH_MASK,
> > + (pcc->dma.addr >> 32) <<
> PCCR5_DMA_ADDRH_SHIFT);
> > + regmap_update_bits(pcc->regmap, PCCR5,
> PCCR5_DMA_LEN_MASK,
> > + (pcc->dma.size / 4) << PCCR5_DMA_LEN_SHIFT);
> > + regmap_update_bits(pcc->regmap, PCCR0,
> > + PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE,
> > + PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE);
> > + } else {
> > + regmap_update_bits(pcc->regmap, PCCR0,
> PCCR0_RX_TRIG_LVL_MASK,
> > + PCC_FIFO_THR_4_EIGHTH <<
> PCCR0_RX_TRIG_LVL_SHIFT);
> > + regmap_update_bits(pcc->regmap, PCCR0,
> > + PCCR0_EN_RX_TMOUT_INT |
> PCCR0_EN_RX_AVAIL_INT,
> > + PCCR0_EN_RX_TMOUT_INT |
> PCCR0_EN_RX_AVAIL_INT);
> > + }
> > +
> > + regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN, PCCR0_EN);
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_pcc_probe(struct platform_device *pdev) {
> > + int rc;
> > + struct aspeed_pcc *pcc;
> > + struct device *dev = &pdev->dev;
> > + uint32_t fifo_size = PAGE_SIZE;
> > +
> > + pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);
>
> Why not dev?
Agree, I will change pcc to dev.
>
> > + if (!pcc)
> > + return -ENOMEM;
> > +
> > + pcc->dev = dev;
> > + rc = of_property_read_u32(dev->of_node, "port-addr", &pcc->port);
>
> Nope
For lpc_snoop node, there is snoop-ports in lpc_snoop to specify which IO port address for use.
In lpc_pcc, the usage is the same. Could I add the pcc-ports property?
>
> > + if (rc) {
> > + dev_err(dev, "cannot get port address\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* optional, by default: 0 -> 1-Byte mode */
> > + of_property_read_u32(dev->of_node, "rec-mode", &pcc->rec_mode);
>
> Nope
OK, I will remove the property.
>
> > + if (!is_valid_rec_mode(pcc->rec_mode)) {
> > + dev_err(dev, "invalid record mode: %u\n",
> > + pcc->rec_mode);
> > + return -EINVAL;
> > + }
> > +
> > + /* optional, by default: 0 -> no don't care bits */
> > + of_property_read_u32(dev->of_node, "port-addr-xbits",
> > +&pcc->port_xbits);
>
> Nope
OK, I will remove the property.
>
> > +
> > + /*
> > + * optional, by default: 0 -> no high address bits
> > + *
> > + * Note that when record mode is set to 1-Byte, this
> > + * property is ignored and the corresponding HW bits
> > + * behave as read/write cycle parser control with the
> > + * value set to 0b11
> > + */
> > + if (pcc->rec_mode) {
> > + of_property_read_u32(dev->of_node, "port-addr-hbits-select",
>
> Nope, none of these properties exist.
OK, I will remove the property.
>
> > + &pcc->port_hbits_select);
> > + if (!is_valid_high_bits_select(pcc->port_hbits_select)) {
> > + dev_err(dev, "invalid high address bits selection: %u\n",
> > + pcc->port_hbits_select);
> > + return -EINVAL;
> > + }
> > + } else {
> > + pcc->port_hbits_select = 0x3;
> > + }
> > +
> > + /* AP note A2600-15 */
> > + pcc->a2600_15 = of_property_read_bool(dev->of_node, "A2600-15");
>
> NAK even more.
OK, I will remove the property.
>
> > + if (pcc->a2600_15)
> > + dev_info(dev, "A2600-15 AP note patch is selected\n");
> > +> + rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> > + if (rc) {
> > + dev_err(dev, "cannot set 64-bits DMA mask\n");
> > + return rc;
> > + }
> > +
> > + pcc->dma_mode = of_property_read_bool(dev->of_node, "dma-mode");
>
> NAK, drop
OK, I will remove the property.
>
> > + if (pcc->dma_mode) {
> > + pcc->dma.size = PCC_DMA_BUFSZ;
> > + pcc->dma.virt = dmam_alloc_coherent(dev,
> > + pcc->dma.size,
> > + &pcc->dma.addr,
> > + GFP_KERNEL);
> > + if (!pcc->dma.virt) {
> > + dev_err(dev, "cannot allocate DMA buffer\n");
> > + return -ENOMEM;
> > + }
> > +
> > + fifo_size = roundup(pcc->dma.size, PAGE_SIZE);
> > + }
> > +
> > + rc = kfifo_alloc(&pcc->fifo, fifo_size, GFP_KERNEL);
> > + if (rc) {
> > + dev_err(dev, "cannot allocate kFIFO\n");
>
> Run coccinelle on your code first.
OK, I will try to run conccinelle by the following link.
https://docs.kernel.org/dev-tools/coccinelle.html
>
> > + return -ENOMEM;
> > + }
> > +
> > + pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > + if (IS_ERR(pcc->regmap)) {
> > + dev_err(dev, "cannot map register\n");
> > + return -ENODEV;
> > + }
> > +
> > + /* Disable PCC and DMA Mode for safety */
> > + regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN |
> > +PCCR0_EN_DMA_MODE, 0);
> > +
> > + /* Clear Rx FIFO. */
> > + regmap_update_bits(pcc->regmap, PCCR0, PCCR0_CLR_RX_FIFO, 1);
> > +
> > + /* Clear All interrupts status. */
> > + regmap_write(pcc->regmap, PCCR2,
> > + PCCR2_INT_STATUS_RX_OVER |
> PCCR2_INT_STATUS_DMA_DONE |
> > + PCCR2_INT_STATUS_PATTERN_A |
> PCCR2_INT_STATUS_PATTERN_B);
> > +
> > + pcc->irq = platform_get_irq(pdev, 0);
> > + if (pcc->irq < 0) {
> > + dev_err(dev, "cannot get IRQ\n");
> > + rc = -ENODEV;
> > + goto err_free_kfifo;
> > + }
> > +
> > + rc = devm_request_irq(dev, pcc->irq, aspeed_pcc_isr, 0, DEVICE_NAME,
> pcc);
> > + if (rc < 0) {
> > + dev_err(dev, "cannot request IRQ handler\n");
> > + goto err_free_kfifo;
> > + }
> > +
> > + init_waitqueue_head(&pcc->wq);
> > +
> > + pcc->mdev_id = ida_alloc(&aspeed_pcc_ida, GFP_KERNEL);
> > + if (pcc->mdev_id < 0) {
> > + dev_err(dev, "cannot allocate ID\n");
> > + return pcc->mdev_id;
> > + }
> > +
> > + pcc->mdev.parent = dev;
> > + pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> > + pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d",
> DEVICE_NAME,
> > + pcc->mdev_id);
> > + pcc->mdev.fops = &pcc_fops;
> > + rc = misc_register(&pcc->mdev);
> > + if (rc) {
> > + dev_err(dev, "cannot register misc device\n");
> > + goto err_free_kfifo;
> > + }
> > +
> > + rc = aspeed_pcc_enable(pcc, dev);
> > + if (rc) {
> > + dev_err(dev, "cannot enable PCC\n");
> > + goto err_dereg_mdev;
> > + }
> > +
> > + dev_set_drvdata(&pdev->dev, pcc);
> > +
> > + dev_info(dev, "module loaded\n");
>
> Drop such function success messages, everywhere.
OK, I will remove the messages.
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists