| Message ID | 20210315095710.7140-5-henning.schild@siemens.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add device drivers for Siemens Industrial PCs | expand |
Am Mon, 15 Mar 2021 10:57:10 +0100 schrieb Henning Schild <henning.schild@siemens.com>: > Siemens industrial PCs unfortunately can not always be properly > identified the way we used to. An earlier commit introduced code that > allows proper identification without looking at DMI strings that could > differ based on product branding. > Switch over to that proper way and revert commits that used to collect > the machines based on unstable strings. > > Fixes: 648e921888ad ("clk: x86: Stop marking clocks as > CLK_IS_CRITICAL") Fixes: e8796c6c69d1 ("platform/x86: pmc_atom: Add > Siemens CONNECT ...") Fixes: f110d252ae79 ("platform/x86: pmc_atom: > Add Siemens SIMATIC ...") Fixes: ad0d315b4d4e ("platform/x86: > pmc_atom: Add Siemens SIMATIC ...") Tested-by: Michael Haener > <michael.haener@siemens.com> Signed-off-by: Henning Schild > <henning.schild@siemens.com> --- > drivers/platform/x86/pmc_atom.c | 47 > +++++++++++++++++++-------------- 1 file changed, 27 insertions(+), > 20 deletions(-) > > diff --git a/drivers/platform/x86/pmc_atom.c > b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29 > 100644 --- a/drivers/platform/x86/pmc_atom.c > +++ b/drivers/platform/x86/pmc_atom.c > @@ -13,6 +13,7 @@ > #include <linux/io.h> > #include <linux/platform_data/x86/clk-pmc-atom.h> > #include <linux/platform_data/x86/pmc_atom.h> > +#include <linux/platform_data/x86/simatic-ipc.h> > #include <linux/platform_device.h> > #include <linux/pci.h> > #include <linux/seq_file.h> > @@ -362,6 +363,23 @@ static void pmc_dbgfs_register(struct pmc_dev > *pmc) } > #endif /* CONFIG_DEBUG_FS */ > > +static bool pmc_clk_is_critical = true; > + > +static int siemens_clk_is_critical(const struct dmi_system_id *d) > +{ > + u32 st_id; > + > + if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &st_id)) > + goto out; > + > + if (st_id == SIMATIC_IPC_IPC227E || st_id == > SIMATIC_IPC_IPC277E) > + return 1; > + > +out: > + pmc_clk_is_critical = false; > + return 1; > +} > + > /* > * Some systems need one or more of their pmc_plt_clks to be > * marked as critical. > @@ -424,24 +442,10 @@ static const struct dmi_system_id > critclk_systems[] = { }, > }, > { > - .ident = "SIMATIC IPC227E", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), > - DMI_MATCH(DMI_PRODUCT_VERSION, "6ES7647-8B"), > - }, > - }, > - { > - .ident = "SIMATIC IPC277E", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), > - DMI_MATCH(DMI_PRODUCT_VERSION, "6AV7882-0"), > - }, > - }, > - { > - .ident = "CONNECT X300", > + .callback = siemens_clk_is_critical, > + .ident = "SIEMENS AG", > .matches = { > DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), > - DMI_MATCH(DMI_PRODUCT_VERSION, > "A5E45074588"), }, > }, > > @@ -453,7 +457,7 @@ static int pmc_setup_clks(struct pci_dev *pdev, > void __iomem *pmc_regmap, { > struct platform_device *clkdev; > struct pmc_clk_data *clk_data; > - const struct dmi_system_id *d = > dmi_first_match(critclk_systems); > + const struct dmi_system_id *d; > > clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); > if (!clk_data) > @@ -461,9 +465,12 @@ static int pmc_setup_clks(struct pci_dev *pdev, > void __iomem *pmc_regmap, > clk_data->base = pmc_regmap; /* offset is added by client */ > clk_data->clks = pmc_data->clks; > - if (d) { > - clk_data->critical = true; > - pr_info("%s critclks quirk enabled\n", d->ident); > + if (dmi_check_system(critclk_systems)) { Had to switch to check_system to get the callback to work. > + clk_data->critical = pmc_clk_is_critical; > + if (clk_data->critical) { > + d = dmi_first_match(critclk_systems); > + pr_info("%s critclks quirk enabled\n", > d->ident); Now need a double match here just to print the ident. Not too happy with that but proposing it like this to keep the ident printing. I guess it could be improved by not printing the ident or having a global variable and global callback to remember the ident to print later. I would propose to not print the ident if the double-match does not find traction. Henning > + } > } > > clkdev = platform_device_register_data(&pdev->dev, > "clk-pmc-atom",
Hi, On 3/15/21 11:14 AM, Henning Schild wrote: > Am Mon, 15 Mar 2021 10:57:10 +0100 > schrieb Henning Schild <henning.schild@siemens.com>: > >> Siemens industrial PCs unfortunately can not always be properly >> identified the way we used to. An earlier commit introduced code that >> allows proper identification without looking at DMI strings that could >> differ based on product branding. >> Switch over to that proper way and revert commits that used to collect >> the machines based on unstable strings. >> >> Fixes: 648e921888ad ("clk: x86: Stop marking clocks as >> CLK_IS_CRITICAL") Fixes: e8796c6c69d1 ("platform/x86: pmc_atom: Add >> Siemens CONNECT ...") Fixes: f110d252ae79 ("platform/x86: pmc_atom: >> Add Siemens SIMATIC ...") Fixes: ad0d315b4d4e ("platform/x86: >> pmc_atom: Add Siemens SIMATIC ...") Tested-by: Michael Haener >> <michael.haener@siemens.com> Signed-off-by: Henning Schild >> <henning.schild@siemens.com> --- >> drivers/platform/x86/pmc_atom.c | 47 >> +++++++++++++++++++-------------- 1 file changed, 27 insertions(+), >> 20 deletions(-) >> >> diff --git a/drivers/platform/x86/pmc_atom.c >> b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29 >> 100644 --- a/drivers/platform/x86/pmc_atom.c >> +++ b/drivers/platform/x86/pmc_atom.c >> @@ -13,6 +13,7 @@ >> #include <linux/io.h> >> #include <linux/platform_data/x86/clk-pmc-atom.h> >> #include <linux/platform_data/x86/pmc_atom.h> >> +#include <linux/platform_data/x86/simatic-ipc.h> >> #include <linux/platform_device.h> >> #include <linux/pci.h> >> #include <linux/seq_file.h> >> @@ -362,6 +363,23 @@ static void pmc_dbgfs_register(struct pmc_dev >> *pmc) } >> #endif /* CONFIG_DEBUG_FS */ >> >> +static bool pmc_clk_is_critical = true; >> + >> +static int siemens_clk_is_critical(const struct dmi_system_id *d) >> +{ >> + u32 st_id; >> + >> + if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &st_id)) >> + goto out; >> + >> + if (st_id == SIMATIC_IPC_IPC227E || st_id == >> SIMATIC_IPC_IPC277E) >> + return 1; >> + >> +out: >> + pmc_clk_is_critical = false; >> + return 1; >> +} >> + >> /* >> * Some systems need one or more of their pmc_plt_clks to be >> * marked as critical. >> @@ -424,24 +442,10 @@ static const struct dmi_system_id >> critclk_systems[] = { }, >> }, >> { >> - .ident = "SIMATIC IPC227E", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, "6ES7647-8B"), >> - }, >> - }, >> - { >> - .ident = "SIMATIC IPC277E", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, "6AV7882-0"), >> - }, >> - }, >> - { >> - .ident = "CONNECT X300", >> + .callback = siemens_clk_is_critical, >> + .ident = "SIEMENS AG", >> .matches = { >> DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, >> "A5E45074588"), }, >> }, >> >> @@ -453,7 +457,7 @@ static int pmc_setup_clks(struct pci_dev *pdev, >> void __iomem *pmc_regmap, { >> struct platform_device *clkdev; >> struct pmc_clk_data *clk_data; >> - const struct dmi_system_id *d = >> dmi_first_match(critclk_systems); >> + const struct dmi_system_id *d; >> >> clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); >> if (!clk_data) >> @@ -461,9 +465,12 @@ static int pmc_setup_clks(struct pci_dev *pdev, >> void __iomem *pmc_regmap, >> clk_data->base = pmc_regmap; /* offset is added by client */ >> clk_data->clks = pmc_data->clks; >> - if (d) { >> - clk_data->critical = true; >> - pr_info("%s critclks quirk enabled\n", d->ident); >> + if (dmi_check_system(critclk_systems)) { > > Had to switch to check_system to get the callback to work. > >> + clk_data->critical = pmc_clk_is_critical; >> + if (clk_data->critical) { >> + d = dmi_first_match(critclk_systems); >> + pr_info("%s critclks quirk enabled\n", >> d->ident); > > Now need a double match here just to print the ident. Not too happy > with that but proposing it like this to keep the ident printing. > > I guess it could be improved by not printing the ident or having a > global variable and global callback to remember the ident to print > later. I would propose to not print the ident if the double-match does > not find traction. IMHO it would be best to add another callback for the non Siemens entries which just prints the ideent and returns 1 to avoid needsly looping over the rest of the array. And then just set the callback member of all the non Siemens entries to this new callback. The space for the callback pointer is already reserved in the struct anyways, so actually setting it does not take up any space. Regards, Hans
Am Mon, 15 Mar 2021 11:19:24 +0100 schrieb Hans de Goede <hdegoede@redhat.com>: > Hi, > > On 3/15/21 11:14 AM, Henning Schild wrote: > > Am Mon, 15 Mar 2021 10:57:10 +0100 > > schrieb Henning Schild <henning.schild@siemens.com>: > > > >> Siemens industrial PCs unfortunately can not always be properly > >> identified the way we used to. An earlier commit introduced code > >> that allows proper identification without looking at DMI strings > >> that could differ based on product branding. > >> Switch over to that proper way and revert commits that used to > >> collect the machines based on unstable strings. > >> > >> Fixes: 648e921888ad ("clk: x86: Stop marking clocks as > >> CLK_IS_CRITICAL") Fixes: e8796c6c69d1 ("platform/x86: pmc_atom: Add > >> Siemens CONNECT ...") Fixes: f110d252ae79 ("platform/x86: pmc_atom: > >> Add Siemens SIMATIC ...") Fixes: ad0d315b4d4e ("platform/x86: > >> pmc_atom: Add Siemens SIMATIC ...") Tested-by: Michael Haener > >> <michael.haener@siemens.com> Signed-off-by: Henning Schild > >> <henning.schild@siemens.com> --- > >> drivers/platform/x86/pmc_atom.c | 47 > >> +++++++++++++++++++-------------- 1 file changed, 27 insertions(+), > >> 20 deletions(-) > >> > >> diff --git a/drivers/platform/x86/pmc_atom.c > >> b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29 > >> 100644 --- a/drivers/platform/x86/pmc_atom.c > >> +++ b/drivers/platform/x86/pmc_atom.c > >> @@ -13,6 +13,7 @@ > >> #include <linux/io.h> > >> #include <linux/platform_data/x86/clk-pmc-atom.h> > >> #include <linux/platform_data/x86/pmc_atom.h> > >> +#include <linux/platform_data/x86/simatic-ipc.h> > >> #include <linux/platform_device.h> > >> #include <linux/pci.h> > >> #include <linux/seq_file.h> > >> @@ -362,6 +363,23 @@ static void pmc_dbgfs_register(struct pmc_dev > >> *pmc) } > >> #endif /* CONFIG_DEBUG_FS */ > >> > >> +static bool pmc_clk_is_critical = true; > >> + > >> +static int siemens_clk_is_critical(const struct dmi_system_id *d) > >> +{ > >> + u32 st_id; > >> + > >> + if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &st_id)) > >> + goto out; > >> + > >> + if (st_id == SIMATIC_IPC_IPC227E || st_id == > >> SIMATIC_IPC_IPC277E) > >> + return 1; > >> + > >> +out: > >> + pmc_clk_is_critical = false; > >> + return 1; > >> +} > >> + > >> /* > >> * Some systems need one or more of their pmc_plt_clks to be > >> * marked as critical. > >> @@ -424,24 +442,10 @@ static const struct dmi_system_id > >> critclk_systems[] = { }, > >> }, > >> { > >> - .ident = "SIMATIC IPC227E", > >> - .matches = { > >> - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), > >> - DMI_MATCH(DMI_PRODUCT_VERSION, > >> "6ES7647-8B"), > >> - }, > >> - }, > >> - { > >> - .ident = "SIMATIC IPC277E", > >> - .matches = { > >> - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), > >> - DMI_MATCH(DMI_PRODUCT_VERSION, > >> "6AV7882-0"), > >> - }, > >> - }, > >> - { > >> - .ident = "CONNECT X300", > >> + .callback = siemens_clk_is_critical, > >> + .ident = "SIEMENS AG", > >> .matches = { > >> DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), > >> - DMI_MATCH(DMI_PRODUCT_VERSION, > >> "A5E45074588"), }, > >> }, > >> > >> @@ -453,7 +457,7 @@ static int pmc_setup_clks(struct pci_dev *pdev, > >> void __iomem *pmc_regmap, { > >> struct platform_device *clkdev; > >> struct pmc_clk_data *clk_data; > >> - const struct dmi_system_id *d = > >> dmi_first_match(critclk_systems); > >> + const struct dmi_system_id *d; > >> > >> clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); > >> if (!clk_data) > >> @@ -461,9 +465,12 @@ static int pmc_setup_clks(struct pci_dev > >> *pdev, void __iomem *pmc_regmap, > >> clk_data->base = pmc_regmap; /* offset is added by client > >> */ clk_data->clks = pmc_data->clks; > >> - if (d) { > >> - clk_data->critical = true; > >> - pr_info("%s critclks quirk enabled\n", d->ident); > >> + if (dmi_check_system(critclk_systems)) { > > > > Had to switch to check_system to get the callback to work. > > > >> + clk_data->critical = pmc_clk_is_critical; > >> + if (clk_data->critical) { > >> + d = dmi_first_match(critclk_systems); > >> + pr_info("%s critclks quirk enabled\n", > >> d->ident); > > > > Now need a double match here just to print the ident. Not too happy > > with that but proposing it like this to keep the ident printing. > > > > I guess it could be improved by not printing the ident or having a > > global variable and global callback to remember the ident to print > > later. I would propose to not print the ident if the double-match > > does not find traction. > > IMHO it would be best to add another callback for the non Siemens > entries which just prints the ideent and returns 1 to avoid needsly > looping over the rest of the array. > > And then just set the callback member of all the non Siemens entries > to this new callback. The space for the callback pointer is already > reserved in the struct anyways, so actually setting it does not take > up any space. Sounds good. I think i will make that another patch on top of the series and send it in reply to "v2 4/4". Maybe squash it in a v3. regards, Henning > Regards, > > Hans >
Hi Henning,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on pavel-linux-leds/for-next]
[also build test WARNING on tip/master linux/master linus/master v5.12-rc3 next-20210315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Henning-Schild/add-device-drivers-for-Siemens-Industrial-PCs/20210315-181441
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: x86_64-randconfig-s022-20210315 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-277-gc089cd2d-dirty
# https://github.com/0day-ci/linux/commit/7b3ce3514b5f314b15ab6e2898104fa3e8e76d59
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Henning-Schild/add-device-drivers-for-Siemens-Industrial-PCs/20210315-181441
git checkout 7b3ce3514b5f314b15ab6e2898104fa3e8e76d59
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
"sparse warnings: (new ones prefixed by >>)"
drivers/platform/x86/pmc_atom.c: note: in included file:
>> include/linux/platform_data/x86/simatic-ipc.h:50:30: sparse: sparse: cast to restricted __le32
vim +50 include/linux/platform_data/x86/simatic-ipc.h
051f5729fd6fb4 Henning Schild 2021-03-15 30
051f5729fd6fb4 Henning Schild 2021-03-15 31 static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
051f5729fd6fb4 Henning Schild 2021-03-15 32 {
051f5729fd6fb4 Henning Schild 2021-03-15 33 u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
051f5729fd6fb4 Henning Schild 2021-03-15 34 int i;
051f5729fd6fb4 Henning Schild 2021-03-15 35 struct {
051f5729fd6fb4 Henning Schild 2021-03-15 36 u8 type; /* type (0xff = binary) */
051f5729fd6fb4 Henning Schild 2021-03-15 37 u8 len; /* len of data entry */
051f5729fd6fb4 Henning Schild 2021-03-15 38 u8 reserved[3];
051f5729fd6fb4 Henning Schild 2021-03-15 39 u32 station_id; /* station id (LE) */
051f5729fd6fb4 Henning Schild 2021-03-15 40 } __packed
051f5729fd6fb4 Henning Schild 2021-03-15 41 *data_entry = (void *)data + sizeof(struct dmi_header);
051f5729fd6fb4 Henning Schild 2021-03-15 42
051f5729fd6fb4 Henning Schild 2021-03-15 43 /* find 4th entry in OEM data */
051f5729fd6fb4 Henning Schild 2021-03-15 44 for (i = 0; i < 3; i++)
051f5729fd6fb4 Henning Schild 2021-03-15 45 data_entry = (void *)((u8 *)(data_entry) + data_entry->len);
051f5729fd6fb4 Henning Schild 2021-03-15 46
051f5729fd6fb4 Henning Schild 2021-03-15 47 /* decode station id */
051f5729fd6fb4 Henning Schild 2021-03-15 48 if (data_entry && (u8 *)data_entry < data + max_len &&
051f5729fd6fb4 Henning Schild 2021-03-15 49 data_entry->type == 0xff && data_entry->len == 9)
051f5729fd6fb4 Henning Schild 2021-03-15 @50 station_id = le32_to_cpu(data_entry->station_id);
051f5729fd6fb4 Henning Schild 2021-03-15 51
051f5729fd6fb4 Henning Schild 2021-03-15 52 return station_id;
051f5729fd6fb4 Henning Schild 2021-03-15 53 }
051f5729fd6fb4 Henning Schild 2021-03-15 54
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -13,6 +13,7 @@ #include <linux/io.h> #include <linux/platform_data/x86/clk-pmc-atom.h> #include <linux/platform_data/x86/pmc_atom.h> +#include <linux/platform_data/x86/simatic-ipc.h> #include <linux/platform_device.h> #include <linux/pci.h> #include <linux/seq_file.h> @@ -362,6 +363,23 @@ static void pmc_dbgfs_register(struct pmc_dev *pmc) } #endif /* CONFIG_DEBUG_FS */ +static bool pmc_clk_is_critical = true; + +static int siemens_clk_is_critical(const struct dmi_system_id *d) +{ + u32 st_id; + + if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &st_id)) + goto out; + + if (st_id == SIMATIC_IPC_IPC227E || st_id == SIMATIC_IPC_IPC277E) + return 1; + +out: + pmc_clk_is_critical = false; + return 1; +} + /* * Some systems need one or more of their pmc_plt_clks to be * marked as critical. @@ -424,24 +442,10 @@ static const struct dmi_system_id critclk_systems[] = { }, }, { - .ident = "SIMATIC IPC227E", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), - DMI_MATCH(DMI_PRODUCT_VERSION, "6ES7647-8B"), - }, - }, - { - .ident = "SIMATIC IPC277E", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), - DMI_MATCH(DMI_PRODUCT_VERSION, "6AV7882-0"), - }, - }, - { - .ident = "CONNECT X300", + .callback = siemens_clk_is_critical, + .ident = "SIEMENS AG", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"), - DMI_MATCH(DMI_PRODUCT_VERSION, "A5E45074588"), }, }, @@ -453,7 +457,7 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, { struct platform_device *clkdev; struct pmc_clk_data *clk_data; - const struct dmi_system_id *d = dmi_first_match(critclk_systems); + const struct dmi_system_id *d; clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); if (!clk_data) @@ -461,9 +465,12 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, clk_data->base = pmc_regmap; /* offset is added by client */ clk_data->clks = pmc_data->clks; - if (d) { - clk_data->critical = true; - pr_info("%s critclks quirk enabled\n", d->ident); + if (dmi_check_system(critclk_systems)) { + clk_data->critical = pmc_clk_is_critical; + if (clk_data->critical) { + d = dmi_first_match(critclk_systems); + pr_info("%s critclks quirk enabled\n", d->ident); + } } clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom",
Siemens industrial PCs unfortunately can not always be properly identified the way we used to. An earlier commit introduced code that allows proper identification without looking at DMI strings that could differ based on product branding. Switch over to that proper way and revert commits that used to collect the machines based on unstable strings. Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") Fixes: e8796c6c69d1 ("platform/x86: pmc_atom: Add Siemens CONNECT ...") Fixes: f110d252ae79 ("platform/x86: pmc_atom: Add Siemens SIMATIC ...") Fixes: ad0d315b4d4e ("platform/x86: pmc_atom: Add Siemens SIMATIC ...") Tested-by: Michael Haener <michael.haener@siemens.com> Signed-off-by: Henning Schild <henning.schild@siemens.com> --- drivers/platform/x86/pmc_atom.c | 47 +++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 20 deletions(-)