[PATCH] staging: unisys: change select to depends for subsystems

[Randy Dunlap]

From: Randy Dunlap 

Drivers should not 'select' a subsystem. Instead they should depend
on it. If the subsystem is disabled, the user probably did that for
a purpose and one driver shouldn't be changing that.

Signed-off-by: Randy Dunlap 
Cc: David Kershner 
Cc: sparmaintai...@unisys.com (Unisys internal)
Cc: de...@driverdev.osuosl.org
Cc: Greg Kroah-Hartman 
---
 drivers/staging/unisys/Kconfig |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- lnx-415-rc8.orig/drivers/staging/unisys/Kconfig
+++ lnx-415-rc8/drivers/staging/unisys/Kconfig
@@ -4,8 +4,7 @@
 menuconfig UNISYSSPAR
bool "Unisys SPAR driver support"
depends on X86_64 && !UML
-   select PCI
-   select ACPI
+   depends on PCI && ACPI
---help---
Support for the Unisys SPAR drivers
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Joe Perches]

On Sun, 2018-01-14 at 20:10 +, Al Viro wrote:
> On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
> > Removed '(' from the end of line, coding style issue.
> 
> The one and only reason for warnings is that they point to
> places more likely to be dodgy.  There is no inherent value
> in having e.g. checkpatch.pl STFU, all wanking about uniformity
> of style nonwithstanding.

[ long and complete response removed, available at
  https://patchwork.kernel.org/patch/10162725/ ]

It was very generous of you to spend so much time on that
informative and thorough reply Al.

My own response was _much_ more terse.

What I wonder is if that sort of guided response can be
setup as documentation for kernel-newbies / janitors so
that future new submitters can have better ideas as to
what code can and should be improved instead of getting
simple and sometimes ill-advised whitespace changes.

cheers, Joe

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 4/7] x86/hyper-v: redirect reenlightment notifications on CPU offlining

[Thomas Gleixner]

On Wed, 13 Dec 2017, Vitaly Kuznetsov wrote:
> +static int hv_cpu_die(unsigned int cpu)
> +{
> + struct hv_reenlightenment_control re_ctrl;
> + int i;
> + static DEFINE_SPINLOCK(lock);
> +
> + if (hv_reenlightenment_cb == NULL)
> + return 0;
> +
> + /* Make sure the CPU we migrate to is not going away too */
> + spin_lock();

What kind of voodoo is this? CPU hotplug is serialized already...

> + rdmsrl(HV_X64_MSR_REENLIGHTENMENT_CONTROL, *((u64 *)_ctrl));
> + if (re_ctrl.target_vp == hv_vp_index[cpu]) {
> + /* Find some other online CPU */
> + for_each_online_cpu(i) {


cpu = cpumask_any_but(cpu_online_mask);

Hmm?

Thanks,

tglx
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/7] x86/hyper-v: reenlightenment notifications support

[Thomas Gleixner]

On Wed, 13 Dec 2017, Vitaly Kuznetsov wrote:
> +void hyperv_reenlightenment_intr(struct pt_regs *regs)

Lacks __visible and __irq_entry annotations.

Thanks,

tglx
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 2/7] x86/hyper-v: add a function to read both TSC and TSC page value simulateneously

[Thomas Gleixner]

On Wed, 13 Dec 2017, Vitaly Kuznetsov wrote:

> This is going to be used from KVM code where we need to get both
> TSC and TSC page value.
> 
> When Hyper-V code is compiled out just return rdtsc(), this will allow us
> to avoid ugly ifdefs in non-Hyper-V code.

That's not what the patch implements

> +static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page 
> *tsc_pg,
> +u64 *cur_tsc)
> +{
> + BUG();
> + return U64_MAX;
> +}

Thanks,

tglx
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/7] x86/hyper-v: check for required priviliges in hyperv_init()

[Thomas Gleixner]

On Wed, 13 Dec 2017, Vitaly Kuznetsov wrote:

> In hyperv_init() we presume we always have access to VP index and hypercall
> MSRs while according to the specification we should check if we're allowed
> to access the corresponding MSRs before accessing them.
> 
> Signed-off-by: Vitaly Kuznetsov 

Reviewed-by: Thomas Gleixner 

> ---
>  arch/x86/hyperv/hv_init.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 189a398290db..21f9d53d9f00 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -110,12 +110,19 @@ static int hv_cpu_init(unsigned int cpu)
>   */
>  void hyperv_init(void)
>  {
> - u64 guest_id;
> + u64 guest_id, required_msrs;
>   union hv_x64_msr_hypercall_contents hypercall_msr;
>  
>   if (x86_hyper_type != X86_HYPER_MS_HYPERV)
>   return;
>  
> + /* Absolutely required MSRs */
> + required_msrs = HV_X64_MSR_HYPERCALL_AVAILABLE |
> + HV_X64_MSR_VP_INDEX_AVAILABLE;
> +
> + if ((ms_hyperv.features & required_msrs) != required_msrs)
> + return;
> +
>   /* Allocate percpu VP index */
>   hv_vp_index = kmalloc_array(num_possible_cpus(), sizeof(*hv_vp_index),
>   GFP_KERNEL);
> -- 
> 2.14.3
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Al Viro]

On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
> Removed '(' from the end of line, coding style issue.

The one and only reason for warnings is that they point to
places more likely to be dodgy.  There is no inherent value
in having e.g. checkpatch.pl STFU, all wanking about uniformity
of style nonwithstanding.

If you end up figuring out how to manipulate some code so that
some warning would go away, something is wrong.  Either the
warning is bogus (which it might very well be - the point of
a warning is "might be worth looking here" and considering
the... level of sophistication of some of those, you can't
expect to get no false alarms) or there *is* something
dodgy in the vicinity.  Dodgy beyond the "this heuristic
triggers", that is.

The first thing to do when dealing with any warning is to
look around and see if it has caught something interesting.
In this case the warning points to excessively long expressions.
With your patch they *still* are just as long, but split in
more unnatural way, making checkpatch.pl miss them.

All of them appear to have the same form - CPHYSADDR applied to
nlm_mmio_base result.  Moreover, looking around that file
shows that all uses of CPHYSADDR and nlm_mmio_base in it
are parts of such expressions.

Most of those expressions are too long and hard to read, so
cleaning them up would probably be a good idea.  And seeing
that composition of these functions keeps occuring in that
sucker, making that composition an inlined helper appears
to be called for.

So
static inline  something( offset)
{
return CPHYSADDR(nlm_mmio_base(offset));
}

and turn those into

gmac4_addr = ioremap(something(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);

void __iomem *gmac0_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET),
   0xfff);
 
gpio_addr = ioremap(something(NETLOGIC_IO_GPIO_OFFSET), 0xfff);

ndata0.mii_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET), 0xfff);

and turn
res->start = CPHYSADDR(nlm_mmio_base(offset));
in xlr_resource_init() into
res->start = something(offset);
as well.  What should the types be?  Result is either fed to ioremap(), or
stored in struct resource 'start' field.  The latter is resource_size_t;
the former varies among the architectures.  Some take resource_size_t,
some - phys_addr_t, some - unsigned long.  On mips (that driver appears to
be mips-only) ioremap takes phys_addr_t; the difference is cosmetical,
anyway, since resource_size_t is typedefed to phys_addr_t on all architectures.

So let it return phys_addr_t.  As for the argument type, looks like it's
an explicit constant or, in case of xlr_resource_init(), an int.  Grepping
shows that all constants involved do fit into int, so we arrive at

static inline phys_addr_t something(int offset)
{
return CPHYSADDR(nlm_mmio_base(offset));
}

probably best placed just before the first use.  OTOH, nlm_mmio_base() is
declared as taking uint32_t, so that might be a better fit for argument.
All constants fed there are between 0 and 2^31, so the only question is
what values does xlr_resource_init() get passed in 'offset' argument.
Callers are
xlr_resource_init(_net1_res[mac * 2],
  xlr_gmac_offsets[mac + 4],
  xlr_gmac_irqs[mac + 4]);
xlr_resource_init(_net0_res[0], xlr_gmac_offsets[0],
  xlr_gmac_irqs[0]);
xlr_resource_init(_net0_res[mac * 2],
  xlr_gmac_offsets[mac],  
xlr_gmac_irqs[mac]);
xlr_resource_init(_net0_res[mac * 2], xlr_gmac_offsets[mac],
  xlr_gmac_irqs[mac]);

so in all cases we have xlr_gmac_offsets[] passed.  What's
going on with that array?  Initialized as
static u32 xlr_gmac_offsets[] = {
NETLOGIC_IO_GMAC_0_OFFSET, NETLOGIC_IO_GMAC_1_OFFSET,
NETLOGIC_IO_GMAC_2_OFFSET, NETLOGIC_IO_GMAC_3_OFFSET,
NETLOGIC_IO_GMAC_4_OFFSET, NETLOGIC_IO_GMAC_5_OFFSET,
NETLOGIC_IO_GMAC_6_OFFSET, NETLOGIC_IO_GMAC_7_OFFSET
};
and never modified, apparently.  OK, that pretty much settles it -
xlr_resource_init() should be getting u32 instead of int.  It's harmless
(the constants here are in the same range), but the things will be easier
for reader that way:

static inline phys_addr_t something(u32 offset)
{
return CPHYSADDR(nlm_mmio_base(offset));
}

static void xlr_resource_init(struct resource *res, u32 offset, int irq)
{
res->name = "gmac";

res->start = something(offset);

While we are at it, 'irq' argument appears to be always taken from
xlr_gmac_irqs[], which is also u32.  So I'd probably do 
static void xlr_resource_init(struct resource *res, u32 offset, u32 irq)
instead.

Now, what do we call that helper?  Definition of CPHYSADDR is
/*
 * Returns the physical address of a 

Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

[Joe Perches]

On Sun, 2018-01-14 at 23:47 +0530, Naveen Panwar wrote:
> Removed '(' from the end of line, coding style issue.
[]
> diff --git a/drivers/staging/netlogic/platform_net.c 
> b/drivers/staging/netlogic/platform_net.c
[]
> @@ -107,8 +107,8 @@ static struct platform_device *gmac_controller2_init(void 
> *gmac0_addr)
>   .dev.platform_data = ,
>   };
>  
> - gmac4_addr = ioremap(CPHYSADDR(
> - nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)), 0xfff);
> + gmac4_addr = ioremap(CPHYSADDR
> + (nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)), 0xfff);

My first reaction is this is ugly.

I suggest
gmac4_addr = 
ioremap(CPHYSADDR(nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET)),
 0xfff);

or using a temporary or a #define for
CPHYSADDR(nlm_mmio_base(NETLOGIC_IO_GMAC_4_OFFSET))

or add a new define like:

#define MMIO_CPHYSADDR(addr) (CPHYSADDR(nlm_mmio_base(addr)))

and change all the netlogic uses of CPHYSADDR so this one
could be

gmac4_addr = ioremap(MMIO_CPHYSADDR(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);

etc...

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: Re: [PATCH] staging: vc04_services: Fix checkpatch.pl warnings

[Sidong Yang]

Hi Stefan,

I'm really glad to review my commit!

I think that your suggestion that changes subject is good.

> i'm okay with the changes, but the subject is too general. We get fixes for 
> checkpach warning nearly once a week. Suggestion:
> 
> staging: vchiq_version: Use tabs for identation
> 
> In case there are more files in vc04_services affected by this particular 
> issue, you are invited to fix them, too.

It's okay to keep working about issue like this? or is there other way to work 
with others?

I'm newbie and don't know how to contribute to linux.

Thanks.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: vc04_services: Fix checkpatch.pl warnings

[Stefan Wahren]

Hi Sidong,

> Sidong Yang  hat am 14. Januar 2018 um 09:11 geschrieben:
> 
> 
> Replace spaces to tabs for indents in beginning of statements.
> 
> Signed-off-by: Sidong Yang 

i'm okay with the changes, but the subject is too general. We get fixes for 
checkpach warning nearly once a week. Suggestion:

staging: vchiq_version: Use tabs for identation

In case there are more files in vc04_services affected by this particular 
issue, you are invited to fix them, too.

Thanks
Stefan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: vc04_services: Fix checkpatch.pl warnings

[Sidong Yang]

Replace spaces to tabs for indents in beginning of statements.

Signed-off-by: Sidong Yang 
---
 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_version.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_version.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_version.c
index 994b81798134..65b2b9966695 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_version.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_version.c
@@ -40,20 +40,20 @@ VC_DEBUG_DECLARE_STRING_VAR(vchiq_build_date,__DATE__);
 
 const char *vchiq_get_build_hostname(void)
 {
-   return vchiq_build_hostname;
+   return vchiq_build_hostname;
 }
 
 const char *vchiq_get_build_version(void)
 {
-   return vchiq_build_version;
+   return vchiq_build_version;
 }
 
 const char *vchiq_get_build_date(void)
 {
-   return vchiq_build_date;
+   return vchiq_build_date;
 }
 
 const char *vchiq_get_build_time(void)
 {
-   return vchiq_build_time;
+   return vchiq_build_time;
 }
-- 
2.11.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel