On Thu, Nov 26, 2020 at 1:51 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi,
please see below for a review.
Am 25.11.20 um 10:09 schrieb KuoHsiang Chou:
[Bug][DP501]
- For security concerning, P2A have to be disabled by CVE regulation.
- FrameBuffer reverses last 2MB used for the image of DP501
- 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-yo...
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@lists.freedesktop.org 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel