On Thu, Nov 26, 2020 at 1:51 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > > Hi, > > please see below for a review. > > Am 25.11.20 um 10:09 schrieb KuoHsiang Chou: > > [Bug][DP501] > > 1. For security concerning, P2A have to be disabled by CVE regulation. > > 2. FrameBuffer reverses last 2MB used for the image of DP501 > > 3. If P2A is disallowed, the default "ioremap()" behavior is non-cached > > and could be an alternative accessing on the image of DP501. > > Please provide a more verbose description of the change. Which problem > does this patch solve? We also need a signed-off-by line per https://dri.freedesktop.org/docs/drm/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin Otherwise we cannot merge a patch. -Daniel > > --- > > drivers/gpu/drm/ast/ast_dp501.c | 131 +++++++++++++++++++++++--------- > > drivers/gpu/drm/ast/ast_drv.h | 2 + > > drivers/gpu/drm/ast/ast_main.c | 12 +++ > > drivers/gpu/drm/ast/ast_mm.c | 1 + > > 4 files changed, 110 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c > > index 88121c0e0d05..7640364ef2bc 100644 > > --- a/drivers/gpu/drm/ast/ast_dp501.c > > +++ b/drivers/gpu/drm/ast/ast_dp501.c > > @@ -189,6 +189,8 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size) > > u32 i, data; > > u32 boot_address; > > > > + if (ast->config_mode != ast_use_p2a) return false; > > + > > The coding style is incorrect. 'Return false' needs to be on the next > line, indented by an additional tab. Here and in other place. > > > > data = ast_mindwm(ast, 0x1e6e2100) & 0x01; > > if (data) { > > boot_address = get_fw_base(ast); > > @@ -207,6 +209,8 @@ static bool ast_launch_m68k(struct drm_device *dev) > > u8 *fw_addr = NULL; > > u8 jreg; > > > > + if (ast->config_mode != ast_use_p2a) return false; > > + > > Coding style. > > > data = ast_mindwm(ast, 0x1e6e2100) & 0x01; > > if (!data) { > > > > @@ -272,24 +276,51 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev) > > u32 boot_address, offset, data; > > u8 linkcap[4], linkrate, linklanes, maxclk = 0xff; > > > > - boot_address = get_fw_base(ast); > > - > > - /* validate FW version */ > > - offset = 0xf000; > > - data = ast_mindwm(ast, boot_address + offset); > > - if ((data & 0xf0) != 0x10) /* version: 1x */ > > - return maxclk; > > - > > - /* Read Link Capability */ > > - offset = 0xf014; > > - *(u32 *)linkcap = ast_mindwm(ast, boot_address + offset); > > - if (linkcap[2] == 0) { > > - linkrate = linkcap[0]; > > - linklanes = linkcap[1]; > > - data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes); > > - if (data > 0xff) > > - data = 0xff; > > - maxclk = (u8)data; > > + if (ast->config_mode == ast_use_p2a) { > > + boot_address = get_fw_base(ast); > > + > > + /* validate FW version */ > > + offset = 0xf000; > > + data = ast_mindwm(ast, boot_address + offset); > > + if ((data & 0xf0) != 0x10) /* version: 1x */ > > + return maxclk; > > Please give these constants some meaningful names. I suggest something like > > #define AST_DP501_FW_VERSION_MASK GENMASK(7, 4) > #define AST_DP501_FW_VERSION_1 BIT(4) > > There are already a few constants in ast_drv.h. I'd put them there as > well. It's better than a comment. > > > + > > + /* Read Link Capability */ > > + offset = 0xf014; > > Please give the offset a meaningful name. > > > > + *(u32 *)linkcap = ast_mindwm(ast, boot_address + offset); > > The cast shoudl go to the right-hand side of the assignment. > > > + if (linkcap[2] == 0) { > > + linkrate = linkcap[0]; > > + linklanes = linkcap[1]; > > + data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes); > > + if (data > 0xff) > > + data = 0xff; > > + maxclk = (u8)data; > > + } > > + } > > + else { > > else goes on the same line as } > > > + if (!ast->reservedbuffer) return 65; /* 1024x768 as default */ > > Coding style. Please give a meaningful name to 65. > > > + > > + /* dummy read */ > > + offset = 0x0000; > > + data = *(u32 *) (ast->reservedbuffer + offset); > > Why is this required? > > reservedbuffer is I/O memory accessed in 32-bit chunks. You should use > readl and writel to access its content. > > > + > > + /* validate FW version */ > > + offset = 0xf000; > > The indention is off. > > > + data = *(u32 *) (ast->reservedbuffer + offset); > > + if ((data & 0xf0) != 0x10) /* version: 1x */ > > + return maxclk; > > Indention. > > > + > > + /* Read Link Capability */ > > + offset = 0xf014; > > + *(u32 *)linkcap = *(u32 *) (ast->reservedbuffer + offset); > > + if (linkcap[2] == 0) { > > + linkrate = linkcap[0]; > > + linklanes = linkcap[1]; > > + data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes); > > + if (data > 0xff) > > + data = 0xff; > > + maxclk = (u8)data; > > + } > > } > > return maxclk; > > } > > @@ -299,25 +330,53 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) > > struct ast_private *ast = to_ast_private(dev); > > u32 i, boot_address, offset, data; > > > > - boot_address = get_fw_base(ast); > > - > > - /* validate FW version */ > > - offset = 0xf000; > > - data = ast_mindwm(ast, boot_address + offset); > > - if ((data & 0xf0) != 0x10) > > - return false; > > - > > - /* validate PnP Monitor */ > > - offset = 0xf010; > > - data = ast_mindwm(ast, boot_address + offset); > > - if (!(data & 0x01)) > > - return false; > > + if (ast->config_mode == ast_use_p2a) { > > + boot_address = get_fw_base(ast); > > > > - /* Read EDID */ > > - offset = 0xf020; > > - for (i = 0; i < 128; i += 4) { > > - data = ast_mindwm(ast, boot_address + offset + i); > > - *(u32 *)(ediddata + i) = data; > > + /* validate FW version */ > > + offset = 0xf000; > > + data = ast_mindwm(ast, boot_address + offset); > > + if ((data & 0xf0) != 0x10) > > + return false; > > + > > + /* validate PnP Monitor */ > > + offset = 0xf010; > > Please name the constant. > > > + data = ast_mindwm(ast, boot_address + offset); > > + if (!(data & 0x01)) > > Please name the constant. > > > + return false; > > + > > + /* Read EDID */ > > + offset = 0xf020; > > + for (i = 0; i < 128; i += 4) { > > + data = ast_mindwm(ast, boot_address + offset + i); > > + *(u32 *)(ediddata + i) = data; > > writel for I/O access > > > + } > > + } > > + else { > > else on wrong line > > > + if (!ast->reservedbuffer) return false; > > + > > + /* dummy read */ > > + offset = 0x0000; > > + data = *(u32 *) (ast->reservedbuffer + offset); > > + > > + /* validate FW version */ > > + offset = 0xf000; > > + data = *(u32 *) (ast->reservedbuffer + offset); > > + if ((data & 0xf0) != 0x10) > > + return false; > > + > > + /* validate PnP Monitor */ > > + offset = 0xf010; > > + data = *(u32 *) (ast->reservedbuffer + offset); > > + if (!(data & 0x01)) > > + return false; > > + > > + /* Read EDID */ > > + offset = 0xf020; > > + for (i = 0; i < 128; i+=4) { > > + data = *(u32 *) (ast->reservedbuffer + offset + i); > > + *(u32 *)(ediddata + i) = data; > > + } > > } > > > > return true; > > diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h > > index 6b9e3b94a712..cd17e0683fd7 100644 > > --- a/drivers/gpu/drm/ast/ast_drv.h > > +++ b/drivers/gpu/drm/ast/ast_drv.h > > @@ -121,12 +121,14 @@ struct ast_private { > > > > void __iomem *regs; > > void __iomem *ioregs; > > + void __iomem *reservedbuffer; > > reservedbuffer has no meaning. As it stores the DP501's firmware, I'd > call it dp501_fw. > > > > > enum ast_chip chip; > > bool vga2_clone; > > uint32_t dram_bus_width; > > uint32_t dram_type; > > uint32_t mclk; > > + uint32_t vram_size; > > > > int fb_mtrr; > > > > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c > > index 4ec6884f6c65..4477b4cf1b06 100644 > > --- a/drivers/gpu/drm/ast/ast_main.c > > +++ b/drivers/gpu/drm/ast/ast_main.c > > @@ -393,6 +393,7 @@ static void ast_device_release(void *data) > > > > /* enable standard VGA decode */ > > ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); > > + pci_iounmap(ast->base.pdev, ast->reservedbuffer); > > } > > > > struct ast_private *ast_device_create(struct drm_driver *drv, > > @@ -449,6 +450,17 @@ struct ast_private *ast_device_create(struct drm_driver *drv, > > if (ret) > > return ERR_PTR(ret); > > > > + /* map reserved buffer */ > > + ast->reservedbuffer = NULL; > > + if (ast->vram_size < pci_resource_len(dev->pdev, 0)) { > > + ast->reservedbuffer = ioremap( \ > > + pci_resource_start(ast->base.pdev, 0) + (unsigned long)ast->vram_size, \ > > + pci_resource_len(dev->pdev, 0) - ast->vram_size); > > Use pci_iomap_range() instead. The function's offset parameter is > vram_size, the function's maxlen parameter is 0. > > You also won't need pci_iounmap(). pci_iomap_range() sets up the cleanup > for you. > > > + if (!ast->reservedbuffer) { > > No braces around single-line branch. > > > + DRM_INFO("failed to map reserved buffer! \n"); > > Use drm_info() instead > > > + } > > + } > > + > > ret = ast_mode_config_init(ast); > > if (ret) > > return ERR_PTR(ret); > > diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c > > index 8392ebde504b..c6fd24493fb3 100644 > > --- a/drivers/gpu/drm/ast/ast_mm.c > > +++ b/drivers/gpu/drm/ast/ast_mm.c > > @@ -90,6 +90,7 @@ int ast_mm_init(struct ast_private *ast) > > int ret; > > > > vram_size = ast_get_vram_size(ast); > > + ast->vram_size = (uint32_t) vram_size; > > You don't need to store vram_size. Look at dev->vram_mm->vram_size instead. > > > > > ret = drmm_vram_helper_init(dev, pci_resource_start(dev->pdev, 0), > > vram_size); > > -- > > 2.18.4 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
