| Message ID | 20210711141002.103445-1-dwaipayanray1@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drivers:edac: Use DEVICE_ATTR helper macros | expand |
On Sun, Jul 11, 2021 at 07:40:02PM +0530, Dwaipayan Ray wrote: > Instead of "open coding" DEVICE_ATTR, use the corresponding > helper macros DEVICE_ATTR_{RW,RO_WO} in amd64_edac.c > I think you meant to write "RO,WO" rather than "RO_WO", correct? Was this change inspired by a code-checking tool or script? > Some function names needed to be changed to match the device > conventions <foo>_show and <foo>_store, but the functionality > itself is unchanged. > > The devices using EDAC_DCT_ATTR_SHOW() are left unchanged. > > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com> > --- > > Changes in v2: > - Revert back the device name changes which broke > the kernel. These were using the macro EDAC_DCT_ATTR_SHOW() > to construct the show methods based on device name. > Reported by Kernel test bot. > > drivers/edac/amd64_edac.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > The $SUBJECT should say something like "EDAC/amd64" since the change is wholly within amd64_edac.c. Using "driver:edac" makes it seem like this patch affects multiple EDAC modules. But otherwise it looks good to me. Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> Thanks, Yazen
On Tue, Jul 13, 2021 at 1:14 AM Yazen Ghannam <yazen.ghannam@amd.com> wrote: > > On Sun, Jul 11, 2021 at 07:40:02PM +0530, Dwaipayan Ray wrote: > > Instead of "open coding" DEVICE_ATTR, use the corresponding > > helper macros DEVICE_ATTR_{RW,RO_WO} in amd64_edac.c > > > > I think you meant to write "RO,WO" rather than "RO_WO", correct? > Yes that's correct. It's a typo. I will fix it. > Was this change inspired by a code-checking tool or script? > Yes, the particular warnings were detected via a checkpatch run on the whole kernel and screening for really unwanted violations. However, the changes were made manually. > > Some function names needed to be changed to match the device > > conventions <foo>_show and <foo>_store, but the functionality > > itself is unchanged. > > > > The devices using EDAC_DCT_ATTR_SHOW() are left unchanged. > > > > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com> > > --- > > > > Changes in v2: > > - Revert back the device name changes which broke > > the kernel. These were using the macro EDAC_DCT_ATTR_SHOW() > > to construct the show methods based on device name. > > Reported by Kernel test bot. > > > > drivers/edac/amd64_edac.c | 21 ++++++++------------- > > 1 file changed, 8 insertions(+), 13 deletions(-) > > > > The $SUBJECT should say something like "EDAC/amd64" since the change is > wholly within amd64_edac.c. Using "driver:edac" makes it seem like this > patch affects multiple EDAC modules. > That makes sense. I will send in a new patch with these updates. > But otherwise it looks good to me. > > Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com> > > Thanks, > Yazen Thanks for the review, Dwaipayan.
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index f0d8f60acee1..99b06a3e8fb1 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -571,8 +571,8 @@ EDAC_DCT_ATTR_SHOW(dbam0); EDAC_DCT_ATTR_SHOW(top_mem); EDAC_DCT_ATTR_SHOW(top_mem2); -static ssize_t hole_show(struct device *dev, struct device_attribute *mattr, - char *data) +static ssize_t dram_hole_show(struct device *dev, struct device_attribute *mattr, + char *data) { struct mem_ctl_info *mci = to_mci(dev); @@ -593,7 +593,7 @@ static DEVICE_ATTR(dhar, S_IRUGO, dhar_show, NULL); static DEVICE_ATTR(dbam, S_IRUGO, dbam0_show, NULL); static DEVICE_ATTR(topmem, S_IRUGO, top_mem_show, NULL); static DEVICE_ATTR(topmem2, S_IRUGO, top_mem2_show, NULL); -static DEVICE_ATTR(dram_hole, S_IRUGO, hole_show, NULL); +static DEVICE_ATTR_RO(dram_hole); static struct attribute *dbg_attrs[] = { &dev_attr_dhar.attr, @@ -802,16 +802,11 @@ static ssize_t inject_write_store(struct device *dev, * update NUM_INJ_ATTRS in case you add new members */ -static DEVICE_ATTR(inject_section, S_IRUGO | S_IWUSR, - inject_section_show, inject_section_store); -static DEVICE_ATTR(inject_word, S_IRUGO | S_IWUSR, - inject_word_show, inject_word_store); -static DEVICE_ATTR(inject_ecc_vector, S_IRUGO | S_IWUSR, - inject_ecc_vector_show, inject_ecc_vector_store); -static DEVICE_ATTR(inject_write, S_IWUSR, - NULL, inject_write_store); -static DEVICE_ATTR(inject_read, S_IWUSR, - NULL, inject_read_store); +static DEVICE_ATTR_RW(inject_section); +static DEVICE_ATTR_RW(inject_word); +static DEVICE_ATTR_RW(inject_ecc_vector); +static DEVICE_ATTR_WO(inject_write); +static DEVICE_ATTR_WO(inject_read); static struct attribute *inj_attrs[] = { &dev_attr_inject_section.attr,
Instead of "open coding" DEVICE_ATTR, use the corresponding helper macros DEVICE_ATTR_{RW,RO_WO} in amd64_edac.c Some function names needed to be changed to match the device conventions <foo>_show and <foo>_store, but the functionality itself is unchanged. The devices using EDAC_DCT_ATTR_SHOW() are left unchanged. Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com> --- Changes in v2: - Revert back the device name changes which broke the kernel. These were using the macro EDAC_DCT_ATTR_SHOW() to construct the show methods based on device name. Reported by Kernel test bot. drivers/edac/amd64_edac.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)