Re: [Outreachy kernel] Re: [PATCH v2 1/5] staging: octeon: remove typedef declaration for cvmx_wqe
> If you're making significant changes to this driver, please test them > using the MIPS cavium_octeon_defconfig which is where this driver is > actually used. > > This driver has broken builds a few times recently which makes me very > tempted to ask that we stop allowing it to be built with COMPILE_TEST. > The whole octeon-stubs.h thing is a horrible hack anyway... Wambui, Please figure out what went wrong here. Are there two sets of typedefs that should have been updated? Others, Would it be reasonable to put the information about how the driver should be compied in the TODO file? git grep cavium_octeon_defconfig in the octeon directory turns up nothing. thanks, julia ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/5] staging: octeon: remove typedef declaration for cvmx_wqe
Hi Wambui, Greg,
On Sat, Oct 12, 2019 at 09:04:31PM +0300, Wambui Karuga wrote:
> Remove typedef declaration from struct cvmx_wqe.
> Also replace its previous uses with new struct declaration.
> Issue found by checkpatch.pl
This may work for x86 builds using COMPILE_TEST that will never actually
run this driver, but it completely breaks the build for the systems that
actually do use the driver...
kernelci.org shows build failures resulting from this patch in
linux-next, and I won't be surprised if other patches in this series
result in similar build breakage too:
https://storage.kernelci.org/next/master/next-20191023/mips/cavium_octeon_defconfig/gcc-8/build.log
If you're making significant changes to this driver, please test them
using the MIPS cavium_octeon_defconfig which is where this driver is
actually used.
This driver has broken builds a few times recently which makes me very
tempted to ask that we stop allowing it to be built with COMPILE_TEST.
The whole octeon-stubs.h thing is a horrible hack anyway...
Thanks,
Paul
>
> Signed-off-by: Wambui Karuga
> ---
> drivers/staging/octeon/ethernet-rx.c | 6 +++---
> drivers/staging/octeon/ethernet-tx.c | 2 +-
> drivers/staging/octeon/ethernet.c | 2 +-
> drivers/staging/octeon/octeon-stubs.h | 22 +++---
> 4 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/octeon/ethernet-rx.c
> b/drivers/staging/octeon/ethernet-rx.c
> index 0e65955c746b..2c16230f993c 100644
> --- a/drivers/staging/octeon/ethernet-rx.c
> +++ b/drivers/staging/octeon/ethernet-rx.c
> @@ -60,7 +60,7 @@ static irqreturn_t cvm_oct_do_interrupt(int irq, void
> *napi_id)
> *
> * Returns Non-zero if the packet can be dropped, zero otherwise.
> */
> -static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
> +static inline int cvm_oct_check_rcv_error(struct cvmx_wqe *work)
> {
> int port;
>
> @@ -135,7 +135,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t
> *work)
> return 0;
> }
>
> -static void copy_segments_to_skb(cvmx_wqe_t *work, struct sk_buff *skb)
> +static void copy_segments_to_skb(struct cvmx_wqe *work, struct sk_buff *skb)
> {
> int segments = work->word2.s.bufs;
> union cvmx_buf_ptr segment_ptr = work->packet_ptr;
> @@ -215,7 +215,7 @@ static int cvm_oct_poll(struct oct_rx_group *rx_group,
> int budget)
> struct sk_buff *skb = NULL;
> struct sk_buff **pskb = NULL;
> int skb_in_hw;
> - cvmx_wqe_t *work;
> + struct cvmx_wqe *work;
> int port;
>
> if (USE_ASYNC_IOBDMA && did_work_request)
> diff --git a/drivers/staging/octeon/ethernet-tx.c
> b/drivers/staging/octeon/ethernet-tx.c
> index c64728fc21f2..a039882e4f70 100644
> --- a/drivers/staging/octeon/ethernet-tx.c
> +++ b/drivers/staging/octeon/ethernet-tx.c
> @@ -515,7 +515,7 @@ int cvm_oct_xmit_pow(struct sk_buff *skb, struct
> net_device *dev)
> void *copy_location;
>
> /* Get a work queue entry */
> - cvmx_wqe_t *work = cvmx_fpa_alloc(CVMX_FPA_WQE_POOL);
> + struct cvmx_wqe *work = cvmx_fpa_alloc(CVMX_FPA_WQE_POOL);
>
> if (unlikely(!work)) {
> printk_ratelimited("%s: Failed to allocate a work queue
> entry\n",
> diff --git a/drivers/staging/octeon/ethernet.c
> b/drivers/staging/octeon/ethernet.c
> index cf8e9a23ebf9..f892f1ad4638 100644
> --- a/drivers/staging/octeon/ethernet.c
> +++ b/drivers/staging/octeon/ethernet.c
> @@ -172,7 +172,7 @@ static void cvm_oct_configure_common_hw(void)
> */
> int cvm_oct_free_work(void *work_queue_entry)
> {
> - cvmx_wqe_t *work = work_queue_entry;
> + struct cvmx_wqe *work = work_queue_entry;
>
> int segments = work->word2.s.bufs;
> union cvmx_buf_ptr segment_ptr = work->packet_ptr;
> diff --git a/drivers/staging/octeon/octeon-stubs.h
> b/drivers/staging/octeon/octeon-stubs.h
> index b2e3c72205dd..7c29cfbd55d1 100644
> --- a/drivers/staging/octeon/octeon-stubs.h
> +++ b/drivers/staging/octeon/octeon-stubs.h
> @@ -183,13 +183,13 @@ union cvmx_buf_ptr {
> } s;
> };
>
> -typedef struct {
> +struct cvmx_wqe {
> union cvmx_wqe_word0 word0;
> union cvmx_wqe_word1 word1;
> union cvmx_pip_wqe_word2 word2;
> union cvmx_buf_ptr packet_ptr;
> uint8_t packet_data[96];
> -} cvmx_wqe_t;
> +};
>
> typedef union {
> uint64_t u64;
> @@ -1198,7 +1198,7 @@ static inline uint64_t cvmx_scratch_read64(uint64_t
> address)
> static inline void cvmx_scratch_write64(uint64_t address, uint64_t value)
> { }
>
> -stati
Re: [PATCH RFC v1 01/12] mm/memory_hotplug: Don't allow to online/offline memory blocks with holes
On 10/22/2019 10:42 PM, David Hildenbrand wrote:
> Our onlining/offlining code is unnecessarily complicated. Only memory
> blocks added during boot can have holes. Hotplugged memory never has
> holes. That memory is already online.
Why hot plugged memory at runtime cannot have holes (e.g a semi bad DIMM).
Currently, do we just abort adding that memory block if there are holes ?
>
> When we stop allowing to offline memory blocks with holes, we implicitly
> stop to online memory blocks with holes.
Reducing hotplug support for memory blocks with holes just to simplify
the code. Is it worth ?
>
> This allows to simplify the code. For example, we no longer have to
> worry about marking pages that fall into memory holes PG_reserved when
> onlining memory. We can stop setting pages PG_reserved.
Could not there be any other way of tracking these holes if not the page
reserved bit. In the memory section itself and corresponding struct pages
just remained poisoned ? Just wondering, might be all wrong here.
>
> Offlining memory blocks added during boot is usually not guranteed to work
> either way. So stopping to do that (if anybody really used and tested
That guarantee does not exist right now because how boot memory could have
been used after boot not from a limitation of the memory hot remove itself.
> this over the years) should not really hurt. For the use case of
> offlining memory to unplug DIMMs, we should see no change. (holes on
> DIMMs would be weird)
Holes on DIMM could be due to HW errors affecting only parts of it. By not
allowing such DIMM's hot add and remove, we are definitely reducing the
scope of overall hotplug functionality. Is code simplification in itself
is worth this reduction in functionality ?
>
> Cc: Andrew Morton
> Cc: Michal Hocko
> Cc: Oscar Salvador
> Cc: Pavel Tatashin
> Cc: Dan Williams
> Signed-off-by: David Hildenbrand
> ---
> mm/memory_hotplug.c | 26 --
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 561371ead39a..7210f4375279 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1447,10 +1447,19 @@ static void node_states_clear_node(int node, struct
> memory_notify *arg)
> node_clear_state(node, N_MEMORY);
> }
>
> +static int count_system_ram_pages_cb(unsigned long start_pfn,
> + unsigned long nr_pages, void *data)
> +{
> + unsigned long *nr_system_ram_pages = data;
> +
> + *nr_system_ram_pages += nr_pages;
> + return 0;
> +}
> +
> static int __ref __offline_pages(unsigned long start_pfn,
> unsigned long end_pfn)
> {
> - unsigned long pfn, nr_pages;
> + unsigned long pfn, nr_pages = 0;
> unsigned long offlined_pages = 0;
> int ret, node, nr_isolate_pageblock;
> unsigned long flags;
> @@ -1461,6 +1470,20 @@ static int __ref __offline_pages(unsigned long
> start_pfn,
>
> mem_hotplug_begin();
>
> + /*
> + * We don't allow to offline memory blocks that contain holes
> + * and consecuently don't allow to online memory blocks that contain
> + * holes. This allows to simplify the code quite a lot and we don't
> + * have to mess with PG_reserved pages for memory holes.
> + */
> + walk_system_ram_range(start_pfn, end_pfn - start_pfn, _pages,
> + count_system_ram_pages_cb);
> + if (nr_pages != end_pfn - start_pfn) {
> + ret = -EINVAL;
> + reason = "memory holes";
> + goto failed_removal;
> + }
> +
> /* This makes hotplug much easier...and readable.
> we assume this for now. .*/
> if (!test_pages_in_a_zone(start_pfn, end_pfn, _start,
> @@ -1472,7 +1495,6 @@ static int __ref __offline_pages(unsigned long
> start_pfn,
>
> zone = page_zone(pfn_to_page(valid_start));
> node = zone_to_nid(zone);
> - nr_pages = end_pfn - start_pfn;
>
> /* set above range as isolated */
> ret = start_isolate_page_range(start_pfn, end_pfn,
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 00/12] mm: Don't mark hotplugged pages PG_reserved (including ZONE_DEVICE)
On 23.10.19 21:39, Dan Williams wrote: > On Wed, Oct 23, 2019 at 10:28 AM David Hildenbrand wrote: >> I dislike this for three reasons a) It does not protect against any races, really, it does not improve things. b) We do have the exact same problem with pfn_to_online_page(). As long as we don't hold the memory hotplug lock, memory can get offlined and remove any time. Racy. >>> >>> True, we need to solve that problem too. That seems to want something >>> lighter weight than the hotplug lock that can be held over pfn lookups >>> + use rather than requiring a page lookup in paths where it's not >>> clear that a page reference would prevent unplug. >>> c) We mix in ZONE specific stuff into the core. It should be "just another zone" >>> >>> Not sure I grok this when the RFC is sprinkling zone-specific >>> is_zone_device_page() throughout the core? >> >> Most users should not care about the zone. pfn_active() would be enough >> in most situations, especially most PFN walkers - "this memmap is valid >> and e.g., contains a valid zone ...". > > Oh, I see, you're saying convert most users to pfn_active() (and some > TBD rcu locking), but only pfn_to_online_page() users would need the > zone lookup? I can get on board with that. I guess my answer to that is simple: If we only care about "is this memmap safe to touch", use pfn_active() (well, with pfn_valid_within() similar as done in pfn_to_online_page() due to memory holes, but these are details - e.g., pfn_active() can check against pfn_valid_within() right away internally). (+locking TBD to make sure it remains active) However, if we want to special case in addition on zones (!ZONE_DEVICE (a.k.a., onlined via memory blocks, managed by the buddy), ZONE_DEVICE, whatever might come in the future, ...), also access the zone stored in the memmap. E.g., by using pfn_to_online_page(). > >> >>> What I propose instead (already discussed in https://lkml.org/lkml/2019/10/10/87) >>> >>> Sorry I missed this earlier... >>> 1. Convert SECTION_IS_ONLINE to SECTION_IS_ACTIVE 2. Convert SECTION_IS_ACTIVE to a subsection bitmap 3. Introduce pfn_active() that checks against the subsection bitmap 4. Once the memmap was initialized / prepared, set the subsection active (similar to SECTION_IS_ONLINE in the buddy right now) 5. Before the memmap gets invalidated, set the subsection inactive (similar to SECTION_IS_ONLINE in the buddy right now) 5. pfn_to_online_page() = pfn_active() && zone != ZONE_DEVICE 6. pfn_to_device_page() = pfn_active() && zone == ZONE_DEVICE >>> >>> This does not seem to reduce any complexity because it still requires >>> a pfn to zone lookup at the end of the process. >>> >>> I.e. converting pfn_to_online_page() to use a new pfn_active() >>> subsection map plus looking up the zone from pfn_to_page() is more >>> steps than just doing a direct pfn to zone lookup. What am I missing? >> >> That a real "pfn to zone" lookup without going via the struct page will >> require to have more than just a single bitmap. IMHO, keeping the >> information at a single place (memmap) is the clean thing to do (not >> replicating it somewhere else). Going via the memmap might not be as >> fast as a direct lookup, but do we actually care? We are already looking >> at "random PFNs we are not sure if there is a valid memmap". > > True, we only care about the validity of the check, and as you pointed > out moving the check to the pfn level does not solve the validity > race. It needs a lock. Let's call pfn_active() "a pfn that is active in the system and has an initialized memmap, which contains sane values" (valid memmap sounds like pfn_valid(), which is actually "there is a memmap which might contain garbage"). Yes we need some sort of lightweight locking as discussed. [...] However, I think we also have to clarify if we need the change in 3 at all. It comes from commit f5509cc18daa7f82bcc553be70df2117c8eedc16 Author: Kees Cook Date: Tue Jun 7 11:05:33 2016 -0700 mm: Hardened usercopy This is the start of porting PAX_USERCOPY into the mainline kernel. This is the first set of features, controlled by CONFIG_HARDENED_USERCOPY. The work is based on code by PaX Team and Brad Spengler, and an earlier port from Casey Schaufler. Additional non-slab page tests are from Rik van Riel. [...] - otherwise, object must not span page allocations (excepting Reserved and CMA ranges) Not sure if we really have to care about ZONE_DEVICE at this point. >>> >>> That check needs to be careful to ignore ZONE_DEVICE pages. There's >>> nothing wrong with a copy spanning ZONE_DEVICE and typical pages. >> >> Please note that the current check would *forbid* this (AFAIKs for a >> single heap object). As discussed in the
[PATCH] Staging: qlge: Rewrite two while loops as simple for loops
This is a task from the TODO list of qlge driver:
- some "while" loops could be rewritten with simple "for"
The change is in functions ql_wait_reg_rdy and ql_wait_cfg in qlge_main.c.
The while loops are basically count based
(they decrement on each iteration),
and it makes more sense to be a for loop construction instead.
Signed-off-by: Samuil Ivanov
---
drivers/staging/qlge/qlge_main.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 0c381d91faa6..6f6b4c06688c 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -167,9 +167,9 @@ void ql_sem_unlock(struct ql_adapter *qdev, u32 sem_mask)
int ql_wait_reg_rdy(struct ql_adapter *qdev, u32 reg, u32 bit, u32 err_bit)
{
u32 temp;
- int count = UDELAY_COUNT;
+ int count;
- while (count) {
+ for (count = 0; count < UDELAY_COUNT; count++) {
temp = ql_read32(qdev, reg);
/* check for errors */
@@ -181,7 +181,6 @@ int ql_wait_reg_rdy(struct ql_adapter *qdev, u32 reg, u32
bit, u32 err_bit)
} else if (temp & bit)
return 0;
udelay(UDELAY_DELAY);
- count--;
}
netif_alert(qdev, probe, qdev->ndev,
"Timed out waiting for reg %x to come ready.\n", reg);
@@ -193,17 +192,16 @@ int ql_wait_reg_rdy(struct ql_adapter *qdev, u32 reg, u32
bit, u32 err_bit)
*/
static int ql_wait_cfg(struct ql_adapter *qdev, u32 bit)
{
- int count = UDELAY_COUNT;
+ int count;
u32 temp;
- while (count) {
+ for (count = 0; count < UDELAY_COUNT; count++) {
temp = ql_read32(qdev, CFG);
if (temp & CFG_LE)
return -EIO;
if (!(temp & bit))
return 0;
udelay(UDELAY_DELAY);
- count--;
}
return -ETIMEDOUT;
}
--
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 00/12] mm: Don't mark hotplugged pages PG_reserved (including ZONE_DEVICE)
On Wed, Oct 23, 2019 at 10:28 AM David Hildenbrand wrote:
>
> >> I dislike this for three reasons
> >>
> >> a) It does not protect against any races, really, it does not improve
> >> things.
> >> b) We do have the exact same problem with pfn_to_online_page(). As long as
> >> we
> >>don't hold the memory hotplug lock, memory can get offlined and remove
> >> any time. Racy.
> >
> > True, we need to solve that problem too. That seems to want something
> > lighter weight than the hotplug lock that can be held over pfn lookups
> > + use rather than requiring a page lookup in paths where it's not
> > clear that a page reference would prevent unplug.
> >
> >> c) We mix in ZONE specific stuff into the core. It should be "just another
> >> zone"
> >
> > Not sure I grok this when the RFC is sprinkling zone-specific
> > is_zone_device_page() throughout the core?
>
> Most users should not care about the zone. pfn_active() would be enough
> in most situations, especially most PFN walkers - "this memmap is valid
> and e.g., contains a valid zone ...".
Oh, I see, you're saying convert most users to pfn_active() (and some
TBD rcu locking), but only pfn_to_online_page() users would need the
zone lookup? I can get on board with that.
>
> >
> >>
> >> What I propose instead (already discussed in
> >> https://lkml.org/lkml/2019/10/10/87)
> >
> > Sorry I missed this earlier...
> >
> >>
> >> 1. Convert SECTION_IS_ONLINE to SECTION_IS_ACTIVE
> >> 2. Convert SECTION_IS_ACTIVE to a subsection bitmap
> >> 3. Introduce pfn_active() that checks against the subsection bitmap
> >> 4. Once the memmap was initialized / prepared, set the subsection active
> >>(similar to SECTION_IS_ONLINE in the buddy right now)
> >> 5. Before the memmap gets invalidated, set the subsection inactive
> >>(similar to SECTION_IS_ONLINE in the buddy right now)
> >> 5. pfn_to_online_page() = pfn_active() && zone != ZONE_DEVICE
> >> 6. pfn_to_device_page() = pfn_active() && zone == ZONE_DEVICE
> >
> > This does not seem to reduce any complexity because it still requires
> > a pfn to zone lookup at the end of the process.
> >
> > I.e. converting pfn_to_online_page() to use a new pfn_active()
> > subsection map plus looking up the zone from pfn_to_page() is more
> > steps than just doing a direct pfn to zone lookup. What am I missing?
>
> That a real "pfn to zone" lookup without going via the struct page will
> require to have more than just a single bitmap. IMHO, keeping the
> information at a single place (memmap) is the clean thing to do (not
> replicating it somewhere else). Going via the memmap might not be as
> fast as a direct lookup, but do we actually care? We are already looking
> at "random PFNs we are not sure if there is a valid memmap".
True, we only care about the validity of the check, and as you pointed
out moving the check to the pfn level does not solve the validity
race. It needs a lock.
>
> >>
> >> Especially, driver-reserved device memory will not get set active in
> >> the subsection bitmap.
> >>
> >> Now to the race. Taking the memory hotplug lock at random places is ugly.
> >> I do
> >> wonder if we can use RCU:
> >
> > Ah, yes, exactly what I was thinking above.
> >
> >>
> >> The user of pfn_active()/pfn_to_online_page()/pfn_to_device_page():
> >>
> >> /* the memmap is guaranteed to remain active under RCU */
> >> rcu_read_lock();
> >> if (pfn_active(random_pfn)) {
> >> page = pfn_to_page(random_pfn);
> >> ... use the page, stays valid
> >> }
> >> rcu_unread_lock();
> >>
> >> Memory offlining/memremap code:
> >>
> >> set_subsections_inactive(pfn, nr_pages); /* clears the bit
> >> atomically */
> >> synchronize_rcu();
> >> /* all users saw the bitmap update, we can invalide the memmap */
> >> remove_pfn_range_from_zone(zone, pfn, nr_pages);
> >
> > Looks good to me.
> >
> >>
> >>>
>
> I only gave it a quick test with DIMMs on x86-64, but didn't test the
> ZONE_DEVICE part at all (any tips for a nice QEMU setup?). Compile-tested
> on x86-64 and PPC.
> >>>
> >>> I'll give it a spin, but I don't think the kernel wants to grow more
> >>> is_zone_device_page() users.
> >>
> >> Let's recap. In this RFC, I introduce a total of 4 (!) users only.
> >> The other parts can rely on pfn_to_online_page() only.
> >>
> >> 1. "staging: kpc2000: Prepare transfer_complete_cb() for PG_reserved
> >> changes"
> >> - Basically never used with ZONE_DEVICE.
> >> - We hold a reference!
> >> - All it protects is a SetPageDirty(page);
> >>
> >> 2. "staging/gasket: Prepare gasket_release_page() for PG_reserved changes"
> >> - Same as 1.
> >>
> >> 3. "mm/usercopy.c: Prepare check_page_span() for PG_reserved changes"
> >> - We come via virt_to_head_page() / virt_to_head_page(), not sure about
> >> references (I assume this should be fine as we don't come via random
> >> PFNs)
> >> - We check that we don't mix Reserved
Re: [Outreachy kernel] [PATCH v2 0/5] Remove typedef declarations in staging: octeon
On Wed, Oct 23, 2019 at 08:43:04PM +0300, Aaro Koskinen wrote: > Hi, > > On Sat, Oct 12, 2019 at 08:35:19PM +0200, Julia Lawall wrote: > > On Sat, 12 Oct 2019, Wambui Karuga wrote: > > > This patchset removes the addition of new typedefs data types in octeon, > > > along with replacing the previous uses with the new declaration format. > > > > > > v2 of the series removes the obsolete "_t" notation in the named types. > > > > > > Wambui Karuga (5): > > > staging: octeon: remove typedef declaration for cvmx_wqe > > > staging: octeon: remove typedef declaration for cvmx_helper_link_info > > > staging: octeon: remove typedef declaration for cvmx_fau_reg_32 > > > staging: octeon: remove typedef declartion for cvmx_pko_command_word0 > > > staging: octeon: remove typedef declaration for cvmx_fau_op_size > > > > > > drivers/staging/octeon/ethernet-mdio.c | 6 +-- > > > drivers/staging/octeon/ethernet-rgmii.c | 4 +- > > > drivers/staging/octeon/ethernet-rx.c | 6 +-- > > > drivers/staging/octeon/ethernet-tx.c | 4 +- > > > drivers/staging/octeon/ethernet.c| 6 +-- > > > drivers/staging/octeon/octeon-ethernet.h | 2 +- > > > drivers/staging/octeon/octeon-stubs.h| 56 > > > 7 files changed, 43 insertions(+), 41 deletions(-) > > > > For the series: > > > > Acked-by: Julia Lawall > > This series breaks the build on MIPS/OCTEON (the only actual HW using this > driver): > > $ make ARCH=mips CROSS_COMPILE=mips64-linux-gnu- cavium_octeon_defconfig > $ make ARCH=mips CROSS_COMPILE=mips64-linux-gnu- > [...] > CC drivers/staging/octeon/ethernet.o > In file included from drivers/staging/octeon/ethernet.c:22: > drivers/staging/octeon/octeon-ethernet.h:94:12: warning: 'union > cvmx_helper_link_info' declared inside parameter list will not be visible > outside of this definition or declaration > union cvmx_helper_link_info li); > ^ > drivers/staging/octeon/ethernet.c: In function 'cvm_oct_free_work': > drivers/staging/octeon/ethernet.c:177:21: error: dereferencing pointer to > incomplete type 'struct cvmx_wqe' > int segments = work->word2.s.bufs; > ^~ > drivers/staging/octeon/ethernet.c: In function 'cvm_oct_common_open': > drivers/staging/octeon/ethernet.c:463:30: error: storage size of 'link_info' > isn't known > union cvmx_helper_link_info link_info; > ^ > > etc. > > Probably all these patches need to be reverted. > > A. Aaro, thanks for the heads up - I can try cross-compiling to see if I can fix the errors. wambui karuga. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v2 0/5] Remove typedef declarations in staging: octeon
Hi, On Sat, Oct 12, 2019 at 08:35:19PM +0200, Julia Lawall wrote: > On Sat, 12 Oct 2019, Wambui Karuga wrote: > > This patchset removes the addition of new typedefs data types in octeon, > > along with replacing the previous uses with the new declaration format. > > > > v2 of the series removes the obsolete "_t" notation in the named types. > > > > Wambui Karuga (5): > > staging: octeon: remove typedef declaration for cvmx_wqe > > staging: octeon: remove typedef declaration for cvmx_helper_link_info > > staging: octeon: remove typedef declaration for cvmx_fau_reg_32 > > staging: octeon: remove typedef declartion for cvmx_pko_command_word0 > > staging: octeon: remove typedef declaration for cvmx_fau_op_size > > > > drivers/staging/octeon/ethernet-mdio.c | 6 +-- > > drivers/staging/octeon/ethernet-rgmii.c | 4 +- > > drivers/staging/octeon/ethernet-rx.c | 6 +-- > > drivers/staging/octeon/ethernet-tx.c | 4 +- > > drivers/staging/octeon/ethernet.c| 6 +-- > > drivers/staging/octeon/octeon-ethernet.h | 2 +- > > drivers/staging/octeon/octeon-stubs.h| 56 > > 7 files changed, 43 insertions(+), 41 deletions(-) > > For the series: > > Acked-by: Julia Lawall This series breaks the build on MIPS/OCTEON (the only actual HW using this driver): $ make ARCH=mips CROSS_COMPILE=mips64-linux-gnu- cavium_octeon_defconfig $ make ARCH=mips CROSS_COMPILE=mips64-linux-gnu- [...] CC drivers/staging/octeon/ethernet.o In file included from drivers/staging/octeon/ethernet.c:22: drivers/staging/octeon/octeon-ethernet.h:94:12: warning: 'union cvmx_helper_link_info' declared inside parameter list will not be visible outside of this definition or declaration union cvmx_helper_link_info li); ^ drivers/staging/octeon/ethernet.c: In function 'cvm_oct_free_work': drivers/staging/octeon/ethernet.c:177:21: error: dereferencing pointer to incomplete type 'struct cvmx_wqe' int segments = work->word2.s.bufs; ^~ drivers/staging/octeon/ethernet.c: In function 'cvm_oct_common_open': drivers/staging/octeon/ethernet.c:463:30: error: storage size of 'link_info' isn't known union cvmx_helper_link_info link_info; ^ etc. Probably all these patches need to be reverted. A. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: octeon: Fix incorrect type in assignment
Hi,
On Thu, Oct 10, 2019 at 07:38:15AM +0300, Wambui Karuga wrote:
> Fix the following warning generated by sparse in
> drivers/staging/octeon/ethernet-tx.c:
>
> drivers/staging/octeon/ethernet-tx.c:563:50: warning: incorrect type in
> assignment (different base types)
> drivers/staging/octeon/ethernet-tx.c:563:50:expected unsigned short
> [usertype] hw_chksum
> drivers/staging/octeon/ethernet-tx.c:563:50:got restricted __wsum
> [usertype] csum
>
> Warning generated by running:
> make C=2 CF="-D__CHECK_ENDIAN__" drivers/staging/octeon/
>
> Signed-off-by: Wambui Karuga
> ---
> drivers/staging/octeon/octeon-stubs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/octeon/octeon-stubs.h
> b/drivers/staging/octeon/octeon-stubs.h
> index 38954b6c89e1..b2e3c72205dd 100644
> --- a/drivers/staging/octeon/octeon-stubs.h
> +++ b/drivers/staging/octeon/octeon-stubs.h
> @@ -123,7 +123,7 @@ union cvmx_pip_wqe_word0 {
> struct {
> uint64_t next_ptr:40;
> uint8_t unused;
> - uint16_t hw_chksum;
> + __wsum hw_chksum;
I don't think this is correct. This struct defines HW register layout,
and the field needs to be 16 bits.
A.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 00/12] mm: Don't mark hotplugged pages PG_reserved (including ZONE_DEVICE)
>> I dislike this for three reasons
>>
>> a) It does not protect against any races, really, it does not improve things.
>> b) We do have the exact same problem with pfn_to_online_page(). As long as we
>>don't hold the memory hotplug lock, memory can get offlined and remove
>> any time. Racy.
>
> True, we need to solve that problem too. That seems to want something
> lighter weight than the hotplug lock that can be held over pfn lookups
> + use rather than requiring a page lookup in paths where it's not
> clear that a page reference would prevent unplug.
>
>> c) We mix in ZONE specific stuff into the core. It should be "just another
>> zone"
>
> Not sure I grok this when the RFC is sprinkling zone-specific
> is_zone_device_page() throughout the core?
Most users should not care about the zone. pfn_active() would be enough
in most situations, especially most PFN walkers - "this memmap is valid
and e.g., contains a valid zone ...".
>
>>
>> What I propose instead (already discussed in
>> https://lkml.org/lkml/2019/10/10/87)
>
> Sorry I missed this earlier...
>
>>
>> 1. Convert SECTION_IS_ONLINE to SECTION_IS_ACTIVE
>> 2. Convert SECTION_IS_ACTIVE to a subsection bitmap
>> 3. Introduce pfn_active() that checks against the subsection bitmap
>> 4. Once the memmap was initialized / prepared, set the subsection active
>>(similar to SECTION_IS_ONLINE in the buddy right now)
>> 5. Before the memmap gets invalidated, set the subsection inactive
>>(similar to SECTION_IS_ONLINE in the buddy right now)
>> 5. pfn_to_online_page() = pfn_active() && zone != ZONE_DEVICE
>> 6. pfn_to_device_page() = pfn_active() && zone == ZONE_DEVICE
>
> This does not seem to reduce any complexity because it still requires
> a pfn to zone lookup at the end of the process.
>
> I.e. converting pfn_to_online_page() to use a new pfn_active()
> subsection map plus looking up the zone from pfn_to_page() is more
> steps than just doing a direct pfn to zone lookup. What am I missing?
That a real "pfn to zone" lookup without going via the struct page will
require to have more than just a single bitmap. IMHO, keeping the
information at a single place (memmap) is the clean thing to do (not
replicating it somewhere else). Going via the memmap might not be as
fast as a direct lookup, but do we actually care? We are already looking
at "random PFNs we are not sure if there is a valid memmap".
>>
>> Especially, driver-reserved device memory will not get set active in
>> the subsection bitmap.
>>
>> Now to the race. Taking the memory hotplug lock at random places is ugly. I
>> do
>> wonder if we can use RCU:
>
> Ah, yes, exactly what I was thinking above.
>
>>
>> The user of pfn_active()/pfn_to_online_page()/pfn_to_device_page():
>>
>> /* the memmap is guaranteed to remain active under RCU */
>> rcu_read_lock();
>> if (pfn_active(random_pfn)) {
>> page = pfn_to_page(random_pfn);
>> ... use the page, stays valid
>> }
>> rcu_unread_lock();
>>
>> Memory offlining/memremap code:
>>
>> set_subsections_inactive(pfn, nr_pages); /* clears the bit
>> atomically */
>> synchronize_rcu();
>> /* all users saw the bitmap update, we can invalide the memmap */
>> remove_pfn_range_from_zone(zone, pfn, nr_pages);
>
> Looks good to me.
>
>>
>>>
I only gave it a quick test with DIMMs on x86-64, but didn't test the
ZONE_DEVICE part at all (any tips for a nice QEMU setup?). Compile-tested
on x86-64 and PPC.
>>>
>>> I'll give it a spin, but I don't think the kernel wants to grow more
>>> is_zone_device_page() users.
>>
>> Let's recap. In this RFC, I introduce a total of 4 (!) users only.
>> The other parts can rely on pfn_to_online_page() only.
>>
>> 1. "staging: kpc2000: Prepare transfer_complete_cb() for PG_reserved changes"
>> - Basically never used with ZONE_DEVICE.
>> - We hold a reference!
>> - All it protects is a SetPageDirty(page);
>>
>> 2. "staging/gasket: Prepare gasket_release_page() for PG_reserved changes"
>> - Same as 1.
>>
>> 3. "mm/usercopy.c: Prepare check_page_span() for PG_reserved changes"
>> - We come via virt_to_head_page() / virt_to_head_page(), not sure about
>> references (I assume this should be fine as we don't come via random
>> PFNs)
>> - We check that we don't mix Reserved (including device memory) and CMA
>> pages when crossing compound pages.
>>
>> I think we can drop 1. and 2., resulting in a total of 2 new users in
>> the same context. I think that is totally tolerable to finally clean
>> this up.
>
> ...but more is_zone_device_page() doesn't "finally clean this up".
> Like we discussed above it's the missing locking that's the real
> cleanup, the pfn_to_online_page() internals are secondary.
It's a different cleanup IMHO. We can't do everything in one shot. But
maybe I can drop the is_zone_device_page() parts from this patch and
completely rely on pfn_to_online_page(). Yes, that
Re: [PATCH] staging: octeon: Remove typedef declaration
Hi,
On Tue, Oct 08, 2019 at 07:09:43AM +0300, Wambui Karuga wrote:
> Fixes checkpatch.pl warning: do not add new typedefs in
> drivers/staging/octeon/octeon-stubs.h:41
>
> Signed-off-by: Wambui Karuga
> ---
> drivers/staging/octeon/octeon-stubs.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/octeon/octeon-stubs.h
> b/drivers/staging/octeon/octeon-stubs.h
> index a4ac3bfb62a8..773591348ef4 100644
> --- a/drivers/staging/octeon/octeon-stubs.h
> +++ b/drivers/staging/octeon/octeon-stubs.h
> @@ -38,7 +38,7 @@
> #define CVMX_NPI_RSL_INT_BLOCKS 0
> #define CVMX_POW_WQ_INT_PC 0
>
> -typedef union {
> +union cvmx_pip_wqe_word2 {
The "real" definition is in arch/mips/include/asm/octeon/cvmx-wqe.h.
octeon-stubs.h is just a temporary hack to allow compile testing without
MIPS cross-compiler. Changing only the stubs file is not really an
improvement.
A.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 00/12] mm: Don't mark hotplugged pages PG_reserved (including ZONE_DEVICE)
On Wed, Oct 23, 2019 at 12:26 AM David Hildenbrand wrote:
>
> On 22.10.19 23:54, Dan Williams wrote:
> > Hi David,
> >
> > Thanks for tackling this!
>
> Thanks for having a look :)
>
> [...]
>
>
> >> I am probably a little bit too careful (but I don't want to break things).
> >> In most places (besides KVM and vfio that are nuts), the
> >> pfn_to_online_page() check could most probably be avoided by a
> >> is_zone_device_page() check. However, I usually get suspicious when I see
> >> a pfn_valid() check (especially after I learned that people mmap parts of
> >> /dev/mem into user space, including memory without memmaps. Also, people
> >> could memmap offline memory blocks this way :/). As long as this does not
> >> hurt performance, I think we should rather do it the clean way.
> >
> > I'm concerned about using is_zone_device_page() in places that are not
> > known to already have a reference to the page. Here's an audit of
> > current usages, and the ones I think need to cleaned up. The "unsafe"
> > ones do not appear to have any protections against the device page
> > being removed (get_dev_pagemap()). Yes, some of these were added by
> > me. The "unsafe? HMM" ones need HMM eyes because HMM leaks device
> > pages into anonymous memory paths and I'm not up to speed on how it
> > guarantees 'struct page' validity vs device shutdown without using
> > get_dev_pagemap().
> >
> > smaps_pmd_entry(): unsafe
> >
> > put_devmap_managed_page(): safe, page reference is held
> >
> > is_device_private_page(): safe? gpu driver manages private page lifetime
> >
> > is_pci_p2pdma_page(): safe, page reference is held
> >
> > uncharge_page(): unsafe? HMM
> >
> > add_to_kill(): safe, protected by get_dev_pagemap() and dax_lock_page()
> >
> > soft_offline_page(): unsafe
> >
> > remove_migration_pte(): unsafe? HMM
> >
> > move_to_new_page(): unsafe? HMM
> >
> > migrate_vma_pages() and helpers: unsafe? HMM
> >
> > try_to_unmap_one(): unsafe? HMM
> >
> > __put_page(): safe
> >
> > release_pages(): safe
> >
> > I'm hoping all the HMM ones can be converted to
> > is_device_private_page() directlly and have that routine grow a nice
> > comment about how it knows it can always safely de-reference its @page
> > argument.
> >
> > For the rest I'd like to propose that we add a facility to determine
> > ZONE_DEVICE by pfn rather than page. The most straightforward why I
> > can think of would be to just add another bitmap to mem_section_usage
> > to indicate if a subsection is ZONE_DEVICE or not.
>
> (it's a somewhat unrelated bigger discussion, but we can start discussing it
> in this thread)
>
> I dislike this for three reasons
>
> a) It does not protect against any races, really, it does not improve things.
> b) We do have the exact same problem with pfn_to_online_page(). As long as we
>don't hold the memory hotplug lock, memory can get offlined and remove any
> time. Racy.
True, we need to solve that problem too. That seems to want something
lighter weight than the hotplug lock that can be held over pfn lookups
+ use rather than requiring a page lookup in paths where it's not
clear that a page reference would prevent unplug.
> c) We mix in ZONE specific stuff into the core. It should be "just another
> zone"
Not sure I grok this when the RFC is sprinkling zone-specific
is_zone_device_page() throughout the core?
>
> What I propose instead (already discussed in
> https://lkml.org/lkml/2019/10/10/87)
Sorry I missed this earlier...
>
> 1. Convert SECTION_IS_ONLINE to SECTION_IS_ACTIVE
> 2. Convert SECTION_IS_ACTIVE to a subsection bitmap
> 3. Introduce pfn_active() that checks against the subsection bitmap
> 4. Once the memmap was initialized / prepared, set the subsection active
>(similar to SECTION_IS_ONLINE in the buddy right now)
> 5. Before the memmap gets invalidated, set the subsection inactive
>(similar to SECTION_IS_ONLINE in the buddy right now)
> 5. pfn_to_online_page() = pfn_active() && zone != ZONE_DEVICE
> 6. pfn_to_device_page() = pfn_active() && zone == ZONE_DEVICE
This does not seem to reduce any complexity because it still requires
a pfn to zone lookup at the end of the process.
I.e. converting pfn_to_online_page() to use a new pfn_active()
subsection map plus looking up the zone from pfn_to_page() is more
steps than just doing a direct pfn to zone lookup. What am I missing?
>
> Especially, driver-reserved device memory will not get set active in
> the subsection bitmap.
>
> Now to the race. Taking the memory hotplug lock at random places is ugly. I do
> wonder if we can use RCU:
Ah, yes, exactly what I was thinking above.
>
> The user of pfn_active()/pfn_to_online_page()/pfn_to_device_page():
>
> /* the memmap is guaranteed to remain active under RCU */
> rcu_read_lock();
> if (pfn_active(random_pfn)) {
> page = pfn_to_page(random_pfn);
> ... use the page, stays valid
> }
> rcu_unread_lock();
>
> Memory
Re: [PATCH RFC v1 02/12] mm/usercopy.c: Prepare check_page_span() for PG_reserved changes
On Wed, Oct 23, 2019 at 10:20:14AM +0200, David Hildenbrand wrote:
> On 22.10.19 19:12, David Hildenbrand wrote:
> > Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
> > change that.
> >
> > Let's make sure that the logic in the function won't change. Once we no
> > longer set these pages to reserved, we can rework this function to
> > perform separate checks for ZONE_DEVICE (split from PG_reserved checks).
> >
> > Cc: Kees Cook
> > Cc: Andrew Morton
> > Cc: Kate Stewart
> > Cc: Allison Randal
> > Cc: "Isaac J. Manjarres"
> > Cc: Qian Cai
> > Cc: Thomas Gleixner
> > Signed-off-by: David Hildenbrand
> > ---
> > mm/usercopy.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index 660717a1ea5c..a3ac4be35cde 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -203,14 +203,15 @@ static inline void check_page_span(const void *ptr,
> > unsigned long n,
> > * device memory), or CMA. Otherwise, reject since the object spans
> > * several independently allocated pages.
> > */
> > - is_reserved = PageReserved(page);
> > + is_reserved = PageReserved(page) || is_zone_device_page(page);
> > is_cma = is_migrate_cma_page(page);
> > if (!is_reserved && !is_cma)
> > usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
> > for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
> > page = virt_to_head_page(ptr);
> > - if (is_reserved && !PageReserved(page))
> > + if (is_reserved && !(PageReserved(page) ||
> > +is_zone_device_page(page)))
> > usercopy_abort("spans Reserved and non-Reserved pages",
> >NULL, to_user, 0, n);
> > if (is_cma && !is_migrate_cma_page(page))
> >
>
> @Kees, would it be okay to stop checking against ZONE_DEVICE pages here or
> is there a good rationale behind this?
>
> (I would turn this patch into a simple update of the comment if we agree
> that we don't care)
There has been work to actually remove the page span checks entirely,
but there wasn't consensus on what the right way forward was. I continue
to leaning toward just dropping it entirely, but Matthew Wilcox has some
alternative ideas that could use some further thought/testing.
--
Kees Cook
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 02/12] mm/usercopy.c: Prepare check_page_span() for PG_reserved changes
On 23.10.19 18:25, Kees Cook wrote:
> On Wed, Oct 23, 2019 at 10:20:14AM +0200, David Hildenbrand wrote:
>> On 22.10.19 19:12, David Hildenbrand wrote:
>>> Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
>>> change that.
>>>
>>> Let's make sure that the logic in the function won't change. Once we no
>>> longer set these pages to reserved, we can rework this function to
>>> perform separate checks for ZONE_DEVICE (split from PG_reserved checks).
>>>
>>> Cc: Kees Cook
>>> Cc: Andrew Morton
>>> Cc: Kate Stewart
>>> Cc: Allison Randal
>>> Cc: "Isaac J. Manjarres"
>>> Cc: Qian Cai
>>> Cc: Thomas Gleixner
>>> Signed-off-by: David Hildenbrand
>>> ---
>>> mm/usercopy.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/usercopy.c b/mm/usercopy.c
>>> index 660717a1ea5c..a3ac4be35cde 100644
>>> --- a/mm/usercopy.c
>>> +++ b/mm/usercopy.c
>>> @@ -203,14 +203,15 @@ static inline void check_page_span(const void *ptr,
>>> unsigned long n,
>>> * device memory), or CMA. Otherwise, reject since the object spans
>>> * several independently allocated pages.
>>> */
>>> - is_reserved = PageReserved(page);
>>> + is_reserved = PageReserved(page) || is_zone_device_page(page);
>>> is_cma = is_migrate_cma_page(page);
>>> if (!is_reserved && !is_cma)
>>> usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
>>> for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
>>> page = virt_to_head_page(ptr);
>>> - if (is_reserved && !PageReserved(page))
>>> + if (is_reserved && !(PageReserved(page) ||
>>> +is_zone_device_page(page)))
>>> usercopy_abort("spans Reserved and non-Reserved pages",
>>>NULL, to_user, 0, n);
>>> if (is_cma && !is_migrate_cma_page(page))
>>>
>>
>> @Kees, would it be okay to stop checking against ZONE_DEVICE pages here or
>> is there a good rationale behind this?
>>
>> (I would turn this patch into a simple update of the comment if we agree
>> that we don't care)
>
> There has been work to actually remove the page span checks entirely,
> but there wasn't consensus on what the right way forward was. I continue
> to leaning toward just dropping it entirely, but Matthew Wilcox has some
> alternative ideas that could use some further thought/testing.
Thanks for your reply!
So, the worst thing that could happen right now, when dropping this
patch, is that we would reject some ranges when hardening is on,
correct? (sounds like that can easily be found by testing if it is
actually relevant)
Do you remember if there were real ZONE_DEVICE usecases that required
this filter to be in place for PG_reserved pages?
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 1/2] staging: sm750fb: format description of parameters in accel.c
Formatting comments in file drivers/staging/sm750fb/sm750_accel.c.
Signed-off-by: Gabriela Bittencourt
---
Changes v2:
- Add name of function at the begining of comment
- Separate each parameter in individuals lines
Here are the commands that I used to test my documentation and the
respective outputs:
$ kdoc_function sm750_accel.c sm750_hw_imageblit man
In NAME:
there is the name of the function, but it's without a brief description
In ARGUMENTS:
argument 'accel' is presented as '-- undescribed --'
$ kdoc_function sm750_accel.c sm750_hw_copyarea man
In NAME:
there is the name of the function, but it's without a brief description
In ARGUMENTS:
argument 'accel' is presented as '-- undescribed --'
$ kernel-doc -none sm750_accel.c
2 Warnings:
sm750_accel.c:155: warning: Function parameter or member 'accel'
not described in 'sm750_hw_copyarea'
sm750_accel.c:321: warning: Function parameter or member 'accel'
not described in 'sm750_hw_imageblit'
I appreciate Randy's explanation about how to test documentation.
Thank you very much.
Changes v3:
- Transforming patch into a patch serie
- Apply similar changes in accel.h
---
drivers/staging/sm750fb/sm750_accel.c | 72 ---
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750_accel.c
b/drivers/staging/sm750fb/sm750_accel.c
index dbcbbd1055da..645813a87490 100644
--- a/drivers/staging/sm750fb/sm750_accel.c
+++ b/drivers/staging/sm750fb/sm750_accel.c
@@ -130,20 +130,28 @@ int sm750_hw_fillrect(struct lynx_accel *accel,
return 0;
}
-int sm750_hw_copyarea(
-struct lynx_accel *accel,
-unsigned int sBase, /* Address of source: offset in frame buffer */
-unsigned int sPitch, /* Pitch value of source surface in BYTE */
-unsigned int sx,
-unsigned int sy, /* Starting coordinate of source surface */
-unsigned int dBase, /* Address of destination: offset in frame buffer */
-unsigned int dPitch, /* Pitch value of destination surface in BYTE */
-unsigned int Bpp,/* Color depth of destination surface */
-unsigned int dx,
-unsigned int dy, /* Starting coordinate of destination surface */
-unsigned int width,
-unsigned int height, /* width and height of rectangle in pixel value */
-unsigned int rop2) /* ROP value */
+/**
+ * sm750_hm_copyarea
+ * @sBase: Address of source: offset in frame buffer
+ * @sPitch: Pitch value of source surface in BYTE
+ * @sx: Starting x coordinate of source surface
+ * @sy: Starting y coordinate of source surface
+ * @dBase: Address of destination: offset in frame buffer
+ * @dPitch: Pitch value of destination surface in BYTE
+ * @Bpp: Color depth of destination surface
+ * @dx: Starting x coordinate of destination surface
+ * @dy: Starting y coordinate of destination surface
+ * @width: width of rectangle in pixel value
+ * @height: height of rectangle in pixel value
+ * @rop2: ROP value
+ */
+int sm750_hw_copyarea(struct lynx_accel *accel,
+ unsigned int sBase, unsigned int sPitch,
+ unsigned int sx, unsigned int sy,
+ unsigned int dBase, unsigned int dPitch,
+ unsigned int Bpp, unsigned int dx, unsigned int dy,
+ unsigned int width, unsigned int height,
+ unsigned int rop2)
{
unsigned int nDirection, de_ctrl;
@@ -288,20 +296,28 @@ static unsigned int deGetTransparency(struct lynx_accel
*accel)
return de_ctrl;
}
-int sm750_hw_imageblit(struct lynx_accel *accel,
-const char *pSrcbuf, /* pointer to start of source buffer in
system memory */
-u32 srcDelta, /* Pitch value (in bytes) of the source
buffer, +ive means top down and -ive mean button up */
-u32 startBit, /* Mono data can start at any bit in a byte,
this value should be 0 to 7 */
-u32 dBase,/* Address of destination: offset in frame
buffer */
-u32 dPitch, /* Pitch value of destination surface in BYTE */
-u32 bytePerPixel, /* Color depth of destination surface */
-u32 dx,
-u32 dy, /* Starting coordinate of destination surface */
-u32 width,
-u32 height, /* width and height of rectangle in pixel value
*/
-u32 fColor, /* Foreground color (corresponding to a 1 in the
monochrome data */
-u32 bColor, /* Background color (corresponding to a 0 in the
monochrome data */
-u32 rop2) /* ROP value */
+/**
+ * sm750_hw_imageblit
+ * @pSrcbuf: pointer to start of source buffer in system memory
+ * @srcDelta: Pitch value (in bytes) of the source buffer, +ive means top down
+ * and -ive mean button up
+ * @startBit: Mono data can start at any bit in a byte, this value should be
+ * 0 to 7
+ * @dBase: Address of destination: offset in frame
[PATCH v3 2/2] staging: sm750fb: format description of parameters in accel.h
Formatting comments in file drivers/staging/sm750fb/sm750_accel.h. Signed-off-by: Gabriela Bittencourt --- Changes v3: - Apply changes in file accel.h --- drivers/staging/sm750fb/sm750_accel.h | 75 --- 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/drivers/staging/sm750fb/sm750_accel.h b/drivers/staging/sm750fb/sm750_accel.h index c4f42002a50f..c16350b5a310 100644 --- a/drivers/staging/sm750fb/sm750_accel.h +++ b/drivers/staging/sm750fb/sm750_accel.h @@ -194,33 +194,50 @@ int sm750_hw_fillrect(struct lynx_accel *accel, u32 x, u32 y, u32 width, u32 height, u32 color, u32 rop); -int sm750_hw_copyarea( -struct lynx_accel *accel, -unsigned int sBase, /* Address of source: offset in frame buffer */ -unsigned int sPitch, /* Pitch value of source surface in BYTE */ -unsigned int sx, -unsigned int sy, /* Starting coordinate of source surface */ -unsigned int dBase, /* Address of destination: offset in frame buffer */ -unsigned int dPitch, /* Pitch value of destination surface in BYTE */ -unsigned int bpp,/* Color depth of destination surface */ -unsigned int dx, -unsigned int dy, /* Starting coordinate of destination surface */ -unsigned int width, -unsigned int height, /* width and height of rectangle in pixel value */ -unsigned int rop2); - -int sm750_hw_imageblit(struct lynx_accel *accel, -const char *pSrcbuf, /* pointer to start of source buffer in system memory */ -u32 srcDelta, /* Pitch value (in bytes) of the source buffer, +ive means top down and -ive mean button up */ -u32 startBit, /* Mono data can start at any bit in a byte, this value should be 0 to 7 */ -u32 dBase,/* Address of destination: offset in frame buffer */ -u32 dPitch, /* Pitch value of destination surface in BYTE */ -u32 bytePerPixel, /* Color depth of destination surface */ -u32 dx, -u32 dy, /* Starting coordinate of destination surface */ -u32 width, -u32 height, /* width and height of rectangle in pixel value */ -u32 fColor, /* Foreground color (corresponding to a 1 in the monochrome data */ -u32 bColor, /* Background color (corresponding to a 0 in the monochrome data */ -u32 rop2); +/** + * sm750_hm_copyarea + * @sBase: Address of source: offset in frame buffer + * @sPitch: Pitch value of source surface in BYTE + * @sx: Starting x coordinate of source surface + * @sy: Starting y coordinate of source surface + * @dBase: Address of destination: offset in frame buffer + * @dPitch: Pitch value of destination surface in BYTE + * @Bpp: Color depth of destination surface + * @dx: Starting x coordinate of destination surface + * @dy: Starting y coordinate of destination surface + * @width: width of rectangle in pixel value + * @height: height of rectangle in pixel value + * @rop2: ROP value + */ +int sm750_hw_copyarea(struct lynx_accel *accel, + unsigned int sBase, unsigned int sPitch, + unsigned int sx, unsigned int sy, + unsigned int dBase, unsigned int dPitch, + unsigned int Bpp, unsigned int dx, unsigned int dy, + unsigned int width, unsigned int height, + unsigned int rop2); + +/** + * sm750_hw_imageblit + * @pSrcbuf: pointer to start of source buffer in system memory + * @srcDelta: Pitch value (in bytes) of the source buffer, +ive means top down + *>- and -ive mean button up + * @startBit: Mono data can start at any bit in a byte, this value should be + *>- 0 to 7 + * @dBase: Address of destination: offset in frame buffer + * @dPitch: Pitch value of destination surface in BYTE + * @bytePerPixel: Color depth of destination surface + * @dx: Starting x coordinate of destination surface + * @dy: Starting y coordinate of destination surface + * @width: width of rectangle in pixel value + * @height: height of rectangle in pixel value + * @fColor: Foreground color (corresponding to a 1 in the monochrome data + * @bColor: Background color (corresponding to a 0 in the monochrome data + * @rop2: ROP value + */ +int sm750_hw_imageblit(struct lynx_accel *accel, const char *pSrcbuf, + u32 srcDelta, u32 startBit, u32 dBase, u32 dPitch, + u32 bytePerPixel, u32 dx, u32 dy, u32 width, + u32 height, u32 fColor, u32 bColor, u32 rop2); + #endif -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 0/2] staging: sm750fb: format description of parameters to the kernel-doc format
Cluster comments that describes parameters of functions and create one single comment before the function in kernel doc format. Gabriela Bittencourt (2): staging: sm750fb: format description of parameters in accel.c staging: sm750fb: format description of parameters in accel.h drivers/staging/sm750fb/sm750_accel.c | 72 +++-- drivers/staging/sm750fb/sm750_accel.h | 75 --- 2 files changed, 90 insertions(+), 57 deletions(-) -- 2.20.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: sm750fb: Format description of parameters the to kernel doc format
Cluster comments that describes parameters of functions and create one
single comment before the function in kernel doc format.
Signed-off-by: Gabriela Bittencourt
---
Changes v2:
- Add name of function at the begining of comment
- Separate each parameter in individuals lines
Here are the commands that I used to test my documentation and the
respective outputs:
$ kdoc_function sm750_acc.c sm750_hw_imageblit man
In NAME:
there is the name of the function, but it's without a brief description
In ARGUMENTS:
argument 'accel' is presented as '-- undescribed --'
$ kdoc_function sm750_acc.c sm750_hw_copyarea man
In NAME:
there is the name of the function, but it's without a brief description
In ARGUMENTS:
argument 'accel' is presented as '-- undescribed --'
$ kernel-doc -none sm750_accel.c
2 Warnings:
sm750_accel.c:155: warning: Function parameter or member 'accel'
not described in 'sm750_hw_copyarea'
sm750_accel.c:321: warning: Function parameter or member 'accel'
not described in 'sm750_hw_imageblit'
I appreciate Randy's explanation about how to test documentation.
Thank you very much.
---
drivers/staging/sm750fb/sm750_accel.c | 72 ---
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750_accel.c
b/drivers/staging/sm750fb/sm750_accel.c
index dbcbbd1055da..645813a87490 100644
--- a/drivers/staging/sm750fb/sm750_accel.c
+++ b/drivers/staging/sm750fb/sm750_accel.c
@@ -130,20 +130,28 @@ int sm750_hw_fillrect(struct lynx_accel *accel,
return 0;
}
-int sm750_hw_copyarea(
-struct lynx_accel *accel,
-unsigned int sBase, /* Address of source: offset in frame buffer */
-unsigned int sPitch, /* Pitch value of source surface in BYTE */
-unsigned int sx,
-unsigned int sy, /* Starting coordinate of source surface */
-unsigned int dBase, /* Address of destination: offset in frame buffer */
-unsigned int dPitch, /* Pitch value of destination surface in BYTE */
-unsigned int Bpp,/* Color depth of destination surface */
-unsigned int dx,
-unsigned int dy, /* Starting coordinate of destination surface */
-unsigned int width,
-unsigned int height, /* width and height of rectangle in pixel value */
-unsigned int rop2) /* ROP value */
+/**
+ * sm750_hm_copyarea
+ * @sBase: Address of source: offset in frame buffer
+ * @sPitch: Pitch value of source surface in BYTE
+ * @sx: Starting x coordinate of source surface
+ * @sy: Starting y coordinate of source surface
+ * @dBase: Address of destination: offset in frame buffer
+ * @dPitch: Pitch value of destination surface in BYTE
+ * @Bpp: Color depth of destination surface
+ * @dx: Starting x coordinate of destination surface
+ * @dy: Starting y coordinate of destination surface
+ * @width: width of rectangle in pixel value
+ * @height: height of rectangle in pixel value
+ * @rop2: ROP value
+ */
+int sm750_hw_copyarea(struct lynx_accel *accel,
+ unsigned int sBase, unsigned int sPitch,
+ unsigned int sx, unsigned int sy,
+ unsigned int dBase, unsigned int dPitch,
+ unsigned int Bpp, unsigned int dx, unsigned int dy,
+ unsigned int width, unsigned int height,
+ unsigned int rop2)
{
unsigned int nDirection, de_ctrl;
@@ -288,20 +296,28 @@ static unsigned int deGetTransparency(struct lynx_accel
*accel)
return de_ctrl;
}
-int sm750_hw_imageblit(struct lynx_accel *accel,
-const char *pSrcbuf, /* pointer to start of source buffer in
system memory */
-u32 srcDelta, /* Pitch value (in bytes) of the source
buffer, +ive means top down and -ive mean button up */
-u32 startBit, /* Mono data can start at any bit in a byte,
this value should be 0 to 7 */
-u32 dBase,/* Address of destination: offset in frame
buffer */
-u32 dPitch, /* Pitch value of destination surface in BYTE */
-u32 bytePerPixel, /* Color depth of destination surface */
-u32 dx,
-u32 dy, /* Starting coordinate of destination surface */
-u32 width,
-u32 height, /* width and height of rectangle in pixel value
*/
-u32 fColor, /* Foreground color (corresponding to a 1 in the
monochrome data */
-u32 bColor, /* Background color (corresponding to a 0 in the
monochrome data */
-u32 rop2) /* ROP value */
+/**
+ * sm750_hw_imageblit
+ * @pSrcbuf: pointer to start of source buffer in system memory
+ * @srcDelta: Pitch value (in bytes) of the source buffer, +ive means top down
+ * and -ive mean button up
+ * @startBit: Mono data can start at any bit in a byte, this value should be
+ * 0 to 7
+ * @dBase: Address of destination: offset in frame buffer
+ * @dPitch: Pitch value of
[PATCH v2] staging: sm750fb: format description of parameters the to kernel doc format
Cluster comments that describes parameters of functions and create one
single comment before the function in kernel doc format.
Signed-off-by: Gabriela Bittencourt
---
Changes v2:
- Add name of function at the begining of comment
- Separate each parameter in individuals lines
Here are the commands that I used to test my documentation and the
respective outputs:
$ kdoc_function sm750_acc.c sm750_hw_imageblit man
In NAME:
there is the name of the function, but it's without a brief description
In ARGUMENTS:
argument 'accel' is presented as '-- undescribed --'
$ kdoc_function sm750_acc.c sm750_hw_copyarea man
In NAME:
there is the name of the function, but it's without a brief description
In ARGUMENTS:
argument 'accel' is presented as '-- undescribed --'
$ kernel-doc -none sm750_accel.c
2 Warnings:
sm750_accel.c:155: warning: Function parameter or member 'accel'
not described in 'sm750_hw_copyarea'
sm750_accel.c:321: warning: Function parameter or member 'accel'
not described in 'sm750_hw_imageblit'
I appreciate Randy's explanation about how to test documentation.
Thank you very much.
---
drivers/staging/sm750fb/sm750_accel.c | 72 ---
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750_accel.c
b/drivers/staging/sm750fb/sm750_accel.c
index dbcbbd1055da..645813a87490 100644
--- a/drivers/staging/sm750fb/sm750_accel.c
+++ b/drivers/staging/sm750fb/sm750_accel.c
@@ -130,20 +130,28 @@ int sm750_hw_fillrect(struct lynx_accel *accel,
return 0;
}
-int sm750_hw_copyarea(
-struct lynx_accel *accel,
-unsigned int sBase, /* Address of source: offset in frame buffer */
-unsigned int sPitch, /* Pitch value of source surface in BYTE */
-unsigned int sx,
-unsigned int sy, /* Starting coordinate of source surface */
-unsigned int dBase, /* Address of destination: offset in frame buffer */
-unsigned int dPitch, /* Pitch value of destination surface in BYTE */
-unsigned int Bpp,/* Color depth of destination surface */
-unsigned int dx,
-unsigned int dy, /* Starting coordinate of destination surface */
-unsigned int width,
-unsigned int height, /* width and height of rectangle in pixel value */
-unsigned int rop2) /* ROP value */
+/**
+ * sm750_hm_copyarea
+ * @sBase: Address of source: offset in frame buffer
+ * @sPitch: Pitch value of source surface in BYTE
+ * @sx: Starting x coordinate of source surface
+ * @sy: Starting y coordinate of source surface
+ * @dBase: Address of destination: offset in frame buffer
+ * @dPitch: Pitch value of destination surface in BYTE
+ * @Bpp: Color depth of destination surface
+ * @dx: Starting x coordinate of destination surface
+ * @dy: Starting y coordinate of destination surface
+ * @width: width of rectangle in pixel value
+ * @height: height of rectangle in pixel value
+ * @rop2: ROP value
+ */
+int sm750_hw_copyarea(struct lynx_accel *accel,
+ unsigned int sBase, unsigned int sPitch,
+ unsigned int sx, unsigned int sy,
+ unsigned int dBase, unsigned int dPitch,
+ unsigned int Bpp, unsigned int dx, unsigned int dy,
+ unsigned int width, unsigned int height,
+ unsigned int rop2)
{
unsigned int nDirection, de_ctrl;
@@ -288,20 +296,28 @@ static unsigned int deGetTransparency(struct lynx_accel
*accel)
return de_ctrl;
}
-int sm750_hw_imageblit(struct lynx_accel *accel,
-const char *pSrcbuf, /* pointer to start of source buffer in
system memory */
-u32 srcDelta, /* Pitch value (in bytes) of the source
buffer, +ive means top down and -ive mean button up */
-u32 startBit, /* Mono data can start at any bit in a byte,
this value should be 0 to 7 */
-u32 dBase,/* Address of destination: offset in frame
buffer */
-u32 dPitch, /* Pitch value of destination surface in BYTE */
-u32 bytePerPixel, /* Color depth of destination surface */
-u32 dx,
-u32 dy, /* Starting coordinate of destination surface */
-u32 width,
-u32 height, /* width and height of rectangle in pixel value
*/
-u32 fColor, /* Foreground color (corresponding to a 1 in the
monochrome data */
-u32 bColor, /* Background color (corresponding to a 0 in the
monochrome data */
-u32 rop2) /* ROP value */
+/**
+ * sm750_hw_imageblit
+ * @pSrcbuf: pointer to start of source buffer in system memory
+ * @srcDelta: Pitch value (in bytes) of the source buffer, +ive means top down
+ * and -ive mean button up
+ * @startBit: Mono data can start at any bit in a byte, this value should be
+ * 0 to 7
+ * @dBase: Address of destination: offset in frame buffer
+ * @dPitch: Pitch value of
Re: [PATCH v8 5/5] media: imx: Try colorimetry at both sink and source pads
Hi Steve,
On Tue 22 Oct 2019 at 17:26, Steve Longerbeam wrote:
> Hi Laurent, Rui,
>
> Besides field type ANY, the imx7 CSI should probably be dealing with other
> field
> type conversions as well. I may be mistaken, but like the imx6, the imx7 does
> not have the ability to detect whether a captured field is a top field or a
> bottom field, so it can't support ALTERNATE mode. It should probably capture
> two
> consecutive fields and in that case and report seq-tb or seq-bt at its output.
> Also the imx6 supports interlacing field lines, if that is the case for imx7
> it
> should also support converting ALTERNATE to INTERLACED at its output.
>
Yeah, that makes sense to me, I already saw yours csi_try_field
that does something in this lines.
I will try to handle that in imx7 also.
Thanks for your inputs here.
Cheers,
Rui
>
> Steve
>
>
> On 10/22/19 6:34 AM, Rui Miguel Silva wrote:
>> Hi Laurent,
>> On Tue 22 Oct 2019 at 02:44, Laurent Pinchart wrote:
>>> Hi Steve,
>>>
>>> On Tue, May 21, 2019 at 06:03:17PM -0700, Steve Longerbeam wrote:
Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters,
renaming it to to imx_media_try_colorimetry(), and call it at both sink and
source pad try_fmt's. The unrelated check for uninitialized field value is
moved out to appropriate places in each subdev try_fmt.
The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited
and full range quantization for both YUV and RGB space, so allow those
for pipelines that route through the IC.
Signed-off-by: Steve Longerbeam
---
Changes in v7:
- squashed with "media: imx: Allow Rec.709 encoding for IC routes".
- remove the RGB full-range quantization restriction for IC routes.
---
drivers/staging/media/imx/imx-ic-prp.c | 6 +-
drivers/staging/media/imx/imx-ic-prpencvf.c | 8 +--
drivers/staging/media/imx/imx-media-csi.c | 19 +++---
drivers/staging/media/imx/imx-media-utils.c | 73 ++---
drivers/staging/media/imx/imx-media-vdic.c | 5 +-
drivers/staging/media/imx/imx-media.h | 5 +-
drivers/staging/media/imx/imx7-media-csi.c | 8 +--
7 files changed, 62 insertions(+), 62 deletions(-)
diff --git a/drivers/staging/media/imx/imx-ic-prp.c
b/drivers/staging/media/imx/imx-ic-prp.c
index 10ffe00f1a54..f87fe0203720 100644
--- a/drivers/staging/media/imx/imx-ic-prp.c
+++ b/drivers/staging/media/imx/imx-ic-prp.c
@@ -193,8 +193,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
sdformat->format.code = cc->codes[0];
}
- imx_media_fill_default_mbus_fields(>format, infmt,
- true);
+ if (sdformat->format.field == V4L2_FIELD_ANY)
+ sdformat->format.field = V4L2_FIELD_NONE;
break;
case PRP_SRC_PAD_PRPENC:
case PRP_SRC_PAD_PRPVF:
@@ -203,6 +203,8 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
break;
}
+ imx_media_try_colorimetry(>format, true);
+
fmt = __prp_get_fmt(priv, cfg, sdformat->pad, sdformat->which);
*fmt = sdformat->format;
out:
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
b/drivers/staging/media/imx/imx-ic-prpencvf.c
index e8b36a181ccc..f2fe3c11c70e 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -907,8 +907,6 @@ static void prp_try_fmt(struct prp_priv *priv,
/* propagate colorimetry from sink */
sdformat->format.colorspace = infmt->colorspace;
sdformat->format.xfer_func = infmt->xfer_func;
- sdformat->format.quantization = infmt->quantization;
- sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
} else {
v4l_bound_align_image(>format.width,
MIN_W_SINK, MAX_W_SINK, W_ALIGN_SINK,
@@ -916,9 +914,11 @@ static void prp_try_fmt(struct prp_priv *priv,
MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
S_ALIGN);
- imx_media_fill_default_mbus_fields(>format, infmt,
- true);
+ if (sdformat->format.field == V4L2_FIELD_ANY)
+ sdformat->format.field = V4L2_FIELD_NONE;
}
+
+ imx_media_try_colorimetry(>format, true);
}
static int prp_set_fmt(struct v4l2_subdev *sd,
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 1d248aca40a9..dce4addadff4 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1375,9
Re: [PATCH -next] staging: comedi: dt2814: remove set but not used variables 'data'
On 23/10/2019 08:48, YueHaibing wrote:
drivers/staging/comedi/drivers/dt2814.c:193:6:
warning: variable data set but not used [-Wunused-but-set-variable]
It is never used, so can be removed.
Signed-off-by: YueHaibing
---
drivers/staging/comedi/drivers/dt2814.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt2814.c
b/drivers/staging/comedi/drivers/dt2814.c
index d2c7157..e7c6787 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -186,21 +186,17 @@ static int dt2814_ai_cmd(struct comedi_device *dev,
struct comedi_subdevice *s)
static irqreturn_t dt2814_interrupt(int irq, void *d)
{
- int lo, hi;
struct comedi_device *dev = d;
struct dt2814_private *devpriv = dev->private;
struct comedi_subdevice *s = dev->read_subdev;
- int data;
if (!dev->attached) {
dev_err(dev->class_dev, "spurious interrupt\n");
return IRQ_HANDLED;
}
- hi = inb(dev->iobase + DT2814_DATA);
- lo = inb(dev->iobase + DT2814_DATA);
-
- data = (hi << 4) | (lo >> 4);
+ inb(dev->iobase + DT2814_DATA);
+ inb(dev->iobase + DT2814_DATA);
if (!(--devpriv->ntrig)) {
int i;
Those variables are currently unused due to a bug that I need to fix.
--
-=( Ian Abbott || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address:)=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] staging: comedi: remove unused variable 'route_table_size'
On 23/10/2019 08:52, YueHaibing wrote: drivers/staging/comedi/drivers/ni_routes.c:52:21: warning: route_table_size defined but not used [-Wunused-const-variable=] It is never used since introduction. Signed-off-by: YueHaibing --- drivers/staging/comedi/drivers/ni_routes.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/ni_routes.c b/drivers/staging/comedi/drivers/ni_routes.c index eb61494..673d732 100644 --- a/drivers/staging/comedi/drivers/ni_routes.c +++ b/drivers/staging/comedi/drivers/ni_routes.c @@ -49,8 +49,6 @@ /* Helper for accessing data. */ #define RVi(table, src, dest) ((table)[(dest) * NI_NUM_NAMES + (src)]) -static const size_t route_table_size = NI_NUM_NAMES * NI_NUM_NAMES; - /* * Find the proper route_values and ni_device_routes tables for this particular * device. Looks good, thanks! Reviewed-by: Ian Abbott -- -=( Ian Abbott || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address:)=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 02/12] mm/usercopy.c: Prepare check_page_span() for PG_reserved changes
On 22.10.19 19:12, David Hildenbrand wrote:
Right now, ZONE_DEVICE memory is always set PG_reserved. We want to
change that.
Let's make sure that the logic in the function won't change. Once we no
longer set these pages to reserved, we can rework this function to
perform separate checks for ZONE_DEVICE (split from PG_reserved checks).
Cc: Kees Cook
Cc: Andrew Morton
Cc: Kate Stewart
Cc: Allison Randal
Cc: "Isaac J. Manjarres"
Cc: Qian Cai
Cc: Thomas Gleixner
Signed-off-by: David Hildenbrand
---
mm/usercopy.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 660717a1ea5c..a3ac4be35cde 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -203,14 +203,15 @@ static inline void check_page_span(const void *ptr,
unsigned long n,
* device memory), or CMA. Otherwise, reject since the object spans
* several independently allocated pages.
*/
- is_reserved = PageReserved(page);
+ is_reserved = PageReserved(page) || is_zone_device_page(page);
is_cma = is_migrate_cma_page(page);
if (!is_reserved && !is_cma)
usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
page = virt_to_head_page(ptr);
- if (is_reserved && !PageReserved(page))
+ if (is_reserved && !(PageReserved(page) ||
+is_zone_device_page(page)))
usercopy_abort("spans Reserved and non-Reserved pages",
NULL, to_user, 0, n);
if (is_cma && !is_migrate_cma_page(page))
@Kees, would it be okay to stop checking against ZONE_DEVICE pages here
or is there a good rationale behind this?
(I would turn this patch into a simple update of the comment if we agree
that we don't care)
--
Thanks,
David / dhildenb
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 06/12] staging/gasket: Prepare gasket_release_page() for PG_reserved changes
On 22.10.19 19:12, David Hildenbrand wrote: Right now, ZONE_DEVICE memory is always set PG_reserved. We want to change that. The pages are obtained via get_user_pages_fast(). I assume, these could be ZONE_DEVICE pages. Let's just exclude them as well explicitly. Cc: Rob Springer Cc: Todd Poynor Cc: Ben Chan Cc: Greg Kroah-Hartman Signed-off-by: David Hildenbrand --- drivers/staging/gasket/gasket_page_table.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/gasket/gasket_page_table.c b/drivers/staging/gasket/gasket_page_table.c index f6d715787da8..d43fed58bf65 100644 --- a/drivers/staging/gasket/gasket_page_table.c +++ b/drivers/staging/gasket/gasket_page_table.c @@ -447,7 +447,7 @@ static bool gasket_release_page(struct page *page) if (!page) return false; - if (!PageReserved(page)) + if (!PageReserved(page) && !is_zone_device_page(page)) SetPageDirty(page); put_page(page); @Dan, is SetPageDirty() on ZONE_DEVICE pages bad or do we simply not care? I think that ending up with ZONE_DEVICE pages here is very unlikely. I'd like to drop this (and the next) patch and document why it is okay to do so. -- Thanks, David / dhildenb ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH -next] staging: comedi: remove unused variable 'route_table_size'
drivers/staging/comedi/drivers/ni_routes.c:52:21: warning: route_table_size defined but not used [-Wunused-const-variable=] It is never used since introduction. Signed-off-by: YueHaibing --- drivers/staging/comedi/drivers/ni_routes.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/ni_routes.c b/drivers/staging/comedi/drivers/ni_routes.c index eb61494..673d732 100644 --- a/drivers/staging/comedi/drivers/ni_routes.c +++ b/drivers/staging/comedi/drivers/ni_routes.c @@ -49,8 +49,6 @@ /* Helper for accessing data. */ #define RVi(table, src, dest) ((table)[(dest) * NI_NUM_NAMES + (src)]) -static const size_t route_table_size = NI_NUM_NAMES * NI_NUM_NAMES; - /* * Find the proper route_values and ni_device_routes tables for this particular * device. -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH -next] staging: comedi: dt2814: remove set but not used variables 'data'
drivers/staging/comedi/drivers/dt2814.c:193:6:
warning: variable data set but not used [-Wunused-but-set-variable]
It is never used, so can be removed.
Signed-off-by: YueHaibing
---
drivers/staging/comedi/drivers/dt2814.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/comedi/drivers/dt2814.c
b/drivers/staging/comedi/drivers/dt2814.c
index d2c7157..e7c6787 100644
--- a/drivers/staging/comedi/drivers/dt2814.c
+++ b/drivers/staging/comedi/drivers/dt2814.c
@@ -186,21 +186,17 @@ static int dt2814_ai_cmd(struct comedi_device *dev,
struct comedi_subdevice *s)
static irqreturn_t dt2814_interrupt(int irq, void *d)
{
- int lo, hi;
struct comedi_device *dev = d;
struct dt2814_private *devpriv = dev->private;
struct comedi_subdevice *s = dev->read_subdev;
- int data;
if (!dev->attached) {
dev_err(dev->class_dev, "spurious interrupt\n");
return IRQ_HANDLED;
}
- hi = inb(dev->iobase + DT2814_DATA);
- lo = inb(dev->iobase + DT2814_DATA);
-
- data = (hi << 4) | (lo >> 4);
+ inb(dev->iobase + DT2814_DATA);
+ inb(dev->iobase + DT2814_DATA);
if (!(--devpriv->ntrig)) {
int i;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v1 00/12] mm: Don't mark hotplugged pages PG_reserved (including ZONE_DEVICE)
On 22.10.19 23:54, Dan Williams wrote:
> Hi David,
>
> Thanks for tackling this!
Thanks for having a look :)
[...]
>> I am probably a little bit too careful (but I don't want to break things).
>> In most places (besides KVM and vfio that are nuts), the
>> pfn_to_online_page() check could most probably be avoided by a
>> is_zone_device_page() check. However, I usually get suspicious when I see
>> a pfn_valid() check (especially after I learned that people mmap parts of
>> /dev/mem into user space, including memory without memmaps. Also, people
>> could memmap offline memory blocks this way :/). As long as this does not
>> hurt performance, I think we should rather do it the clean way.
>
> I'm concerned about using is_zone_device_page() in places that are not
> known to already have a reference to the page. Here's an audit of
> current usages, and the ones I think need to cleaned up. The "unsafe"
> ones do not appear to have any protections against the device page
> being removed (get_dev_pagemap()). Yes, some of these were added by
> me. The "unsafe? HMM" ones need HMM eyes because HMM leaks device
> pages into anonymous memory paths and I'm not up to speed on how it
> guarantees 'struct page' validity vs device shutdown without using
> get_dev_pagemap().
>
> smaps_pmd_entry(): unsafe
>
> put_devmap_managed_page(): safe, page reference is held
>
> is_device_private_page(): safe? gpu driver manages private page lifetime
>
> is_pci_p2pdma_page(): safe, page reference is held
>
> uncharge_page(): unsafe? HMM
>
> add_to_kill(): safe, protected by get_dev_pagemap() and dax_lock_page()
>
> soft_offline_page(): unsafe
>
> remove_migration_pte(): unsafe? HMM
>
> move_to_new_page(): unsafe? HMM
>
> migrate_vma_pages() and helpers: unsafe? HMM
>
> try_to_unmap_one(): unsafe? HMM
>
> __put_page(): safe
>
> release_pages(): safe
>
> I'm hoping all the HMM ones can be converted to
> is_device_private_page() directlly and have that routine grow a nice
> comment about how it knows it can always safely de-reference its @page
> argument.
>
> For the rest I'd like to propose that we add a facility to determine
> ZONE_DEVICE by pfn rather than page. The most straightforward why I
> can think of would be to just add another bitmap to mem_section_usage
> to indicate if a subsection is ZONE_DEVICE or not.
(it's a somewhat unrelated bigger discussion, but we can start discussing it in
this thread)
I dislike this for three reasons
a) It does not protect against any races, really, it does not improve things.
b) We do have the exact same problem with pfn_to_online_page(). As long as we
don't hold the memory hotplug lock, memory can get offlined and remove any
time. Racy.
c) We mix in ZONE specific stuff into the core. It should be "just another zone"
What I propose instead (already discussed in
https://lkml.org/lkml/2019/10/10/87)
1. Convert SECTION_IS_ONLINE to SECTION_IS_ACTIVE
2. Convert SECTION_IS_ACTIVE to a subsection bitmap
3. Introduce pfn_active() that checks against the subsection bitmap
4. Once the memmap was initialized / prepared, set the subsection active
(similar to SECTION_IS_ONLINE in the buddy right now)
5. Before the memmap gets invalidated, set the subsection inactive
(similar to SECTION_IS_ONLINE in the buddy right now)
5. pfn_to_online_page() = pfn_active() && zone != ZONE_DEVICE
6. pfn_to_device_page() = pfn_active() && zone == ZONE_DEVICE
Especially, driver-reserved device memory will not get set active in
the subsection bitmap.
Now to the race. Taking the memory hotplug lock at random places is ugly. I do
wonder if we can use RCU:
The user of pfn_active()/pfn_to_online_page()/pfn_to_device_page():
/* the memmap is guaranteed to remain active under RCU */
rcu_read_lock();
if (pfn_active(random_pfn)) {
page = pfn_to_page(random_pfn);
... use the page, stays valid
}
rcu_unread_lock();
Memory offlining/memremap code:
set_subsections_inactive(pfn, nr_pages); /* clears the bit atomically */
synchronize_rcu();
/* all users saw the bitmap update, we can invalide the memmap */
remove_pfn_range_from_zone(zone, pfn, nr_pages);
>
>>
>> I only gave it a quick test with DIMMs on x86-64, but didn't test the
>> ZONE_DEVICE part at all (any tips for a nice QEMU setup?). Compile-tested
>> on x86-64 and PPC.
>
> I'll give it a spin, but I don't think the kernel wants to grow more
> is_zone_device_page() users.
Let's recap. In this RFC, I introduce a total of 4 (!) users only.
The other parts can rely on pfn_to_online_page() only.
1. "staging: kpc2000: Prepare transfer_complete_cb() for PG_reserved changes"
- Basically never used with ZONE_DEVICE.
- We hold a reference!
- All it protects is a SetPageDirty(page);
2. "staging/gasket: Prepare gasket_release_page() for PG_reserved changes"
- Same as 1.
3. "mm/usercopy.c: Prepare check_page_span() for PG_reserved changes"
- We
