Re: [PATCH RFC PKS/PMEM 05/58] kmap: Introduce k[un]map_thread
On Mon, Nov 09 2020 at 20:59, Ira Weiny wrote: > On Tue, Nov 10, 2020 at 02:13:56AM +0100, Thomas Gleixner wrote: > Also, we can convert the new memcpy_*_page() calls to kmap_local() as well. > [For now my patch just uses kmap_atomic().] > > I've not looked at all of the patches in your latest version. Have you > included converting any of the kmap() call sites? I thought you were more > focused on converting the kmap_atomic() to kmap_local()? I did not touch any of those yet, but it's a logical consequence to convert all kmap() instances which are _not_ creating a global mapping over to it. Thanks, tglx ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] drivers: most: add ALSA sound driver
On Fri, Nov 06, 2020 at 05:30:54PM +0100, Christian Gromm wrote:
> +static struct list_head adpt_list;
> +
> +#define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> +SNDRV_PCM_INFO_MMAP_VALID | \
> +SNDRV_PCM_INFO_BATCH | \
> +SNDRV_PCM_INFO_INTERLEAVED | \
> +SNDRV_PCM_INFO_BLOCK_TRANSFER)
> +
> +static void swap_copy16(u16 *dest, const u16 *source, unsigned int bytes)
> +{
> + unsigned int i = 0;
> +
> + while (i < (bytes / 2)) {
> + dest[i] = swab16(source[i]);
> + i++;
> + }
> +}
> +
> +static void swap_copy24(u8 *dest, const u8 *source, unsigned int bytes)
> +{
> + unsigned int i = 0;
> +
> + while (i < bytes - 2) {
Can bytes ever be zero? If so then this will corrupt memory and crash.
Generally "int i;" is less risky than "unsigned int i;". Of course, I
recently almost introduced a situation where we were copying up to
ULONG_MAX bytes so there are times when iterators should be size_t so
that does happen. It could be buggy either way is what I'm saying but
generally "unsigned int i;" is more often buggy.
> + dest[i] = source[i + 2];
> + dest[i + 1] = source[i + 1];
> + dest[i + 2] = source[i];
> + i += 3;
> + }
> +}
> +
> +static void swap_copy32(u32 *dest, const u32 *source, unsigned int bytes)
> +{
> + unsigned int i = 0;
> +
> + while (i < bytes / 4) {
> + dest[i] = swab32(source[i]);
> + i++;
> + }
> +}
> +
> +static void alsa_to_most_memcpy(void *alsa, void *most, unsigned int bytes)
> +{
> + memcpy(most, alsa, bytes);
> +}
> +
> +static void alsa_to_most_copy16(void *alsa, void *most, unsigned int bytes)
> +{
> + swap_copy16(most, alsa, bytes);
> +}
> +
> +static void alsa_to_most_copy24(void *alsa, void *most, unsigned int bytes)
> +{
> + swap_copy24(most, alsa, bytes);
> +}
> +
> +static void alsa_to_most_copy32(void *alsa, void *most, unsigned int bytes)
> +{
> + swap_copy32(most, alsa, bytes);
> +}
> +
> +static void most_to_alsa_memcpy(void *alsa, void *most, unsigned int bytes)
> +{
> + memcpy(alsa, most, bytes);
> +}
> +
> +static void most_to_alsa_copy16(void *alsa, void *most, unsigned int bytes)
> +{
> + swap_copy16(alsa, most, bytes);
> +}
> +
> +static void most_to_alsa_copy24(void *alsa, void *most, unsigned int bytes)
> +{
> + swap_copy24(alsa, most, bytes);
> +}
> +
> +static void most_to_alsa_copy32(void *alsa, void *most, unsigned int bytes)
> +{
> + swap_copy32(alsa, most, bytes);
> +}
> +
> +/**
> + * get_channel - get pointer to channel
> + * @iface: interface structure
> + * @channel_id: channel ID
> + *
> + * This traverses the channel list and returns the channel matching the
> + * ID and interface.
> + *
> + * Returns pointer to channel on success or NULL otherwise.
> + */
> +static struct channel *get_channel(struct most_interface *iface,
> +int channel_id)
> +{
> + struct sound_adapter *adpt = iface->priv;
> + struct channel *channel, *tmp;
> +
> + list_for_each_entry_safe(channel, tmp, &adpt->dev_list, list) {
> + if ((channel->iface == iface) && (channel->id == channel_id))
> + return channel;
No need to use the _safe() version of this loop macro. You're not
freeing anything. My concern is that sometimes people think the _safe()
has something to do with locking and it does not.
> + }
> + return NULL;
> +}
> +
[ Snip ]
> +/**
> + * audio_probe_channel - probe function of the driver module
> + * @iface: pointer to interface instance
> + * @channel_id: channel index/ID
> + * @cfg: pointer to actual channel configuration
> + * @arg_list: string that provides the name of the device to be created in
> /dev
> + * plus the desired audio resolution
> + *
> + * Creates sound card, pcm device, sets pcm ops and registers sound card.
> + *
> + * Returns 0 on success or error code otherwise.
> + */
> +static int audio_probe_channel(struct most_interface *iface, int channel_id,
> +struct most_channel_config *cfg,
> +char *device_name, char *arg_list)
> +{
> + struct channel *channel;
> + struct sound_adapter *adpt;
> + struct snd_pcm *pcm;
> + int playback_count = 0;
> + int capture_count = 0;
> + int ret;
> + int direction;
> + u16 ch_num;
> + char *sample_res;
> + char arg_list_cpy[STRING_SIZE];
> +
> + if (cfg->data_type != MOST_CH_SYNC) {
> + pr_err("Incompatible channel type\n");
> + return -EINVAL;
> + }
> + strlcpy(arg_list_cpy, arg_list, STRING_SIZE);
> + ret = split_arg_list(arg_list_cpy, &ch_num, &sample_res);
> + if (ret < 0)
> + return ret;
> +
> + list_for_each_entry(adpt, &adpt_list, list) {
> + if (adpt->iface != iface)
> + continue;
> +
Re: [PATCH v4 0/4] MT7621 PCIe PHY
Hi,
On Sat, Oct 31, 2020 at 1:22 PM Sergio Paracuellos
wrote:
>
> This series adds support for the PCIe PHY found in the Mediatek
> MT7621 SoC.
>
> This is the first attempt to get feedback of what is missing in
> this driver to be promoted from staging.
>
> There is also a 'mt7621-pci' driver which is the controller part
> which is still in staging and is a client of this phy.
>
> Both drivers have been tested together in a gnubee1 board.
>
> This series are rebased on the top of linux-next:
> commit 4e78c578cb98 ("Add linux-next specific files for 20201030")
>
> Changes in v4:
> - Bindings moved from txt to yaml so previous Rob's Reviewed-by
> is not in the new patch with the yaml file.
> - 'phy-cells' property means now if phy is dual-ported.
> - Avoid custom 'xlate' function and properly set registers
> when the phy is dual ported.
> - Add use of 'builtin_platform_driver'.
> - Added a patch including myself as maintainer in the
> MAINTAINERS file.
> - Add a patch removing patch from staging to make easier
> the complete inclusion and avoid possible problems might
> appear in 'linux-next' if the series are included.
Kishon, Vinod, any comments on this? Is here something wrong or
missing in order to get this accepted through your tree?
Thanks in advance for your time.
Best regards,
Sergio Paracuellos
>
> Changes in v3:
> - Recollect Rob's Reviewed-by of bindings.
> - Make Kishon Vijay suggested changes in v2:
> (See https://lkml.org/lkml/2019/4/17/53)
> - Kconfig:
> * Add depends on COMPILE_TEST
> * Select REGMAP_MMIO
> - Make use of 'soc_device_attribute' and 'soc_device_match'
> - Use regmap mmio API instead of directly 'readl' and 'writel'.
> - Use 'platform_get_resource' instead of 'of_address_to_resource'.
>
> Changes in v2:
> - Reorder patches to get bindings first in the series.
> - Don't use child nodes in the device tree. Use #phy-cells=1 instead.
> - Update driver code with new 'xlate' function for the new device tree.
> - Minor changes in driver's macros changing some spaces to tabs.
>
> Thanks in advance for your time.
>
> Best regards,
> Sergio Paracuellos
>
> Sergio Paracuellos (4):
> dt-bindings: phy: Add binding for Mediatek MT7621 PCIe PHY
> phy: ralink: Add PHY driver for MT7621 PCIe PHY
> MAINTAINERS: add MT7621 PHY PCI maintainer
> staging: mt7621-pci-phy: remove driver from staging
>
> .../devicetree/bindings/phy}/mediatek,mt7621-pci-phy.yaml | 0
> MAINTAINERS | 6 ++
> drivers/phy/ralink/Kconfig| 8
> drivers/phy/ralink/Makefile | 1 +
> .../pci-mt7621-phy.c => phy/ralink/phy-mt7621-pci.c} | 0
> drivers/staging/Kconfig | 2 --
> drivers/staging/Makefile | 1 -
> drivers/staging/mt7621-pci-phy/Kconfig| 8
> drivers/staging/mt7621-pci-phy/Makefile | 2 --
> drivers/staging/mt7621-pci-phy/TODO | 4
> 10 files changed, 15 insertions(+), 17 deletions(-)
> rename {drivers/staging/mt7621-pci-phy =>
> Documentation/devicetree/bindings/phy}/mediatek,mt7621-pci-phy.yaml (100%)
> rename drivers/{staging/mt7621-pci-phy/pci-mt7621-phy.c =>
> phy/ralink/phy-mt7621-pci.c} (100%)
> delete mode 100644 drivers/staging/mt7621-pci-phy/Kconfig
> delete mode 100644 drivers/staging/mt7621-pci-phy/Makefile
> delete mode 100644 drivers/staging/mt7621-pci-phy/TODO
>
> --
> 2.25.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 01/11] firmware: raspberrypi: Introduce devm_rpi_firmware_get()
Hi Bartosz, thanks for the feedback.
On Thu, 2020-11-05 at 10:42 +0100, Bartosz Golaszewski wrote:
> On Thu, Nov 5, 2020 at 10:28 AM Nicolas Saenz Julienne
> wrote:
> > Hi Bartosz, thanks for the review.
> >
> > On Thu, 2020-11-05 at 10:13 +0100, Bartosz Golaszewski wrote:
> > > > +/**
> > > > + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure.
> > > > + * @firmware_node:Pointer to the firmware Device Tree node.
> > > > + *
> > > > + * Returns NULL is the firmware device is not ready.
> > > > + */
> > > > +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev,
> > > > + struct device_node
> > > > *firmware_node)
> > > > +{
> > > > + struct platform_device *pdev =
> > > > of_find_device_by_node(firmware_node);
> > > > + struct rpi_firmware *fw;
> > > > +
> > > > + if (!pdev)
> > > > + return NULL;
> > > > +
> > > > + fw = platform_get_drvdata(pdev);
> > > > + if (!fw)
> > > > + return NULL;
> > > > +
> > > > + if (!refcount_inc_not_zero(&fw->consumers))
> > > > + return NULL;
> > > > +
> > > > + if (devm_add_action_or_reset(dev, rpi_firmware_put, fw))
> > > > + return NULL;
> > > > +
> > > > + return fw;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
> > >
> > > Usually I'd expect the devres variant to simply call
> > > rpi_firmware_get() and then schedule a release callback which would
> > > call whatever function is the release counterpart for it currently.
> > > Devres actions are for drivers which want to schedule some more
> > > unusual tasks at driver detach. Any reason for designing it this way?
> >
> > Yes, see patch #8 where I get rid of rpi_firmware_get() altogether after
> > converting all users to devres. Since there is no use for the vanilla
> > version
> > of the function anymore, I figured it'd be better to merge everything into
> > devm_rpi_firmware_get(). That said it's not something I have strong feelings
> > about.
> >
>
> I see. So the previous version didn't really have any reference
> counting and it leaked the reference returned by
> of_find_device_by_node(), got it. Could you just clarify for me the
> logic behind the wait_queue in rpi_firmware_remove()? If the firmware
> driver gets detached and remove() stops on the wait_queue - it will be
> stuck until the last user releases the firmware. I'm not sure this is
> correct.
Yes, that's what I meant to implement.
> I'd prefer to see a kref with a release callback and remove
> would simply decrease the kref and return. Each user would do the same
> and then after the last user is detached the firmware would be
> destroyed.
Sounds good to me. I'll update it.
> Don't we really have some centralized firmware subsystem that would handle
> this?
Sadly no, this is an RPi specific thing, it doesn't live behind a standard like
other firmware based protocols (for ex. SCMI), and evolves as the needs arise.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/11] Introduce Simple atomic counters
On Fri, Oct 16, 2020 at 03:51:25PM -0700, Kees Cook wrote: > On Fri, Oct 16, 2020 at 12:53:13PM +0200, Peter Zijlstra wrote: > > That's like saying: "I'm too lazy to track what I've looked at already". > > You're basically proposing to graffiti "Kees was here -- 16/10/2020" all > > over the kernel. Just so you can see where you still need to go. > > > > It says the code was (assuming your audit was correct) good at that > > date, but has no guarantees for any moment after that. > > That kind of bit-rot marking is exactly what I would like to avoid: just > putting a comment in is pointless. Making the expectations of the usage > become _enforced_ is the goal. And having it enforced by the _compiler_ > is key. Just adding a meaningless attribute that a static checker > will notice some time and hope people fix them doesn't scale either > (just look at how many sparse warnings there are). Most Sparse warnings are false positives. People do actually fix the ones which matter. I think this patchset could be useful. I'm working on a refcounting check for Smatch. I want to warn about when we forget to drop a reference on an error path. Right now I just assume that anything with "error", "drop" or "->stats->" in the name is just a counter. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 11/13] drivers/staging/rtl8188eu: convert stats to use seqnum_ops
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN when it overflows and
should not be used to guard resource lifetimes, device usage and
open counts that control state changes, and pm states.
atomic_t variables used for stats are atomic counters. Overflow will
wrap around and reset the stats and no change with the conversion.
Convert them to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().
Signed-off-by: Shuah Khan
---
drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 23 ++-
.../staging/rtl8188eu/include/rtw_mlme_ext.h | 3 ++-
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index b3eddcb83cd0..31dd9d31dd9a 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -7,6 +7,7 @@
#define _RTW_MLME_EXT_C_
#include
+#include
#include
#include
@@ -3860,7 +3861,7 @@ static void init_mlme_ext_priv_value(struct adapter
*padapter)
_12M_RATE_, _24M_RATE_, 0xff,
};
- atomic_set(&pmlmeext->event_seq, 0);
+ seqnum32_set(&pmlmeext->event_seq, 0);
pmlmeext->mgnt_seq = 0;/* reset to zero when disconnect at client mode
*/
pmlmeext->cur_channel = padapter->registrypriv.channel;
@@ -4189,7 +4190,9 @@ void report_survey_event(struct adapter *padapter,
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct survey_event);
pc2h_evt_hdr->ID = _Survey_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
psurvey_evt = (struct survey_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
@@ -4239,7 +4242,9 @@ void report_surveydone_event(struct adapter *padapter)
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct surveydone_event);
pc2h_evt_hdr->ID = _SurveyDone_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
psurveydone_evt = (struct surveydone_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
psurveydone_evt->bss_cnt = pmlmeext->sitesurvey_res.bss_cnt;
@@ -4283,7 +4288,9 @@ void report_join_res(struct adapter *padapter, int res)
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct joinbss_event);
pc2h_evt_hdr->ID = _JoinBss_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
pjoinbss_evt = (struct joinbss_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
memcpy((unsigned char *)(&pjoinbss_evt->network.network),
&pmlmeinfo->network, sizeof(struct wlan_bssid_ex));
@@ -4333,7 +4340,9 @@ void report_del_sta_event(struct adapter *padapter,
unsigned char *MacAddr,
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct stadel_event);
pc2h_evt_hdr->ID = _DelSTA_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
pdel_sta_evt = (struct stadel_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
ether_addr_copy((unsigned char *)(&pdel_sta_evt->macaddr), MacAddr);
@@ -4386,7 +4395,9 @@ void report_add_sta_event(struct adapter *padapter,
unsigned char *MacAddr,
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct stassoc_event);
pc2h_evt_hdr->ID = _AddSTA_EVT_;
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
padd_sta_evt = (struct stassoc_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
ether_addr_copy((unsigned char *)(&padd_sta_evt->macaddr), MacAddr);
diff --git a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
index b11a6886a083..09b7e3bb2738 100644
--- a/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8188eu/include/rtw_mlme_ext.h
@@ -7,6 +7,7 @@
#ifndef __RTW_MLME_EXT_H_
#define __RTW_MLME_EXT_H_
+#include
#include
#include
#include
@@ -393,7 +394,7 @@ struct p2p_oper_class_map {
struct mlme_ext_priv {
struct adapter *padapter;
[PATCH 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN when it overflows and
should not be used to guard resource lifetimes, device usage and
open counts that control state changes, and pm states.
atomic_t variables used for error_count and ios_threshold are atomic
counters and guarded by max. values. No change to the behavior with
this change.
Signed-off-by: Shuah Khan
---
.../staging/unisys/visorhba/visorhba_main.c | 37 ++-
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
b/drivers/staging/unisys/visorhba/visorhba_main.c
index 7ae5306b92fe..3209958b8aaa 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
struct visordisk_info {
struct scsi_device *sdev;
u32 valid;
- atomic_t ios_threshold;
- atomic_t error_count;
+ struct seqnum32 ios_threshold;
+ struct seqnum32 error_count;
struct visordisk_info *next;
};
@@ -374,10 +375,10 @@ static int visorhba_abort_handler(struct scsi_cmnd
*scsicmd)
scsidev = scsicmd->device;
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
- atomic_inc(&vdisk->error_count);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+ seqnum32_inc(&vdisk->error_count);
else
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
rtn = forward_taskmgmt_command(TASK_MGMT_ABORT_TASK, scsidev);
if (rtn == SUCCESS) {
scsicmd->result = DID_ABORT << 16;
@@ -401,10 +402,10 @@ static int visorhba_device_reset_handler(struct scsi_cmnd
*scsicmd)
scsidev = scsicmd->device;
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
- atomic_inc(&vdisk->error_count);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+ seqnum32_inc(&vdisk->error_count);
else
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
rtn = forward_taskmgmt_command(TASK_MGMT_LUN_RESET, scsidev);
if (rtn == SUCCESS) {
scsicmd->result = DID_RESET << 16;
@@ -429,10 +430,10 @@ static int visorhba_bus_reset_handler(struct scsi_cmnd
*scsicmd)
scsidev = scsicmd->device;
shost_for_each_device(scsidev, scsidev->host) {
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
- atomic_inc(&vdisk->error_count);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT)
+ seqnum32_inc(&vdisk->error_count);
else
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ seqnum32_set(&vdisk->ios_threshold,
IOS_ERROR_THRESHOLD);
}
rtn = forward_taskmgmt_command(TASK_MGMT_BUS_RESET, scsidev);
if (rtn == SUCCESS) {
@@ -803,9 +804,9 @@ static void do_scsi_linuxstat(struct uiscmdrsp *cmdrsp,
return;
/* Okay see what our error_count is here */
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT) {
- atomic_inc(&vdisk->error_count);
- atomic_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
+ if (seqnum32_read(&vdisk->error_count) < VISORHBA_ERROR_COUNT) {
+ seqnum32_inc(&vdisk->error_count);
+ seqnum32_set(&vdisk->ios_threshold, IOS_ERROR_THRESHOLD);
}
}
@@ -881,10 +882,10 @@ static void do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp,
kfree(buf);
} else {
vdisk = scsidev->hostdata;
- if (atomic_read(&vdisk->ios_threshold) > 0) {
- atomic_dec(&vdisk->ios_threshold);
- if (atomic_read(&vdisk->ios_threshold) == 0)
- atomic_set(&vdisk->error_count, 0);
+ if (seqnum32_read(&vdisk->ios_threshold) > 0) {
+ seqnum32_dec(&vdisk->ios_threshold);
+ if (seqnum32_read(&vdisk->ios_threshold) == 0)
+ seqnum32_set(&vdisk->error_count, 0);
}
}
}
--
2.27.0
___
devel mailing list
de...@linuxdri
[PATCH 09/13] drivers/staging/rtl8723bs: convert stats to use seqnum_ops
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN when it overflows and
should not be used to guard resource lifetimes, device usage and
open counts that control state changes, and pm states.
atomic_t variables used for stats are atomic counters. Overflow will
wrap around and reset the stats and no change with the conversion.
Convert them to use seqnum_ops. This conversion replaces inc_return()
with _inc() followed by _read().
Signed-off-by: Shuah Khan
---
drivers/staging/rtl8723bs/core/rtw_cmd.c | 3 +-
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 33 +--
drivers/staging/rtl8723bs/include/rtw_cmd.h | 3 +-
.../staging/rtl8723bs/include/rtw_mlme_ext.h | 3 +-
4 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 2abe205e3453..c3350f97816d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
static struct _cmd_callback rtw_cmd_callback[] = {
{GEN_CMD_CODE(_Read_MACREG), NULL}, /*0*/
@@ -207,7 +208,7 @@ static void c2h_wk_callback(_workitem * work);
int rtw_init_evt_priv(struct evt_priv *pevtpriv)
{
/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
- atomic_set(&pevtpriv->event_seq, 0);
+ seqnum32_set(&pevtpriv->event_seq, 0);
pevtpriv->evt_done_cnt = 0;
_init_workitem(&pevtpriv->c2h_wk, c2h_wk_callback, NULL);
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index b912ad2f4b72..addcd11b153b 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
static struct mlme_handler mlme_sta_tbl[] = {
@@ -281,7 +282,7 @@ static void init_mlme_ext_priv_value(struct adapter
*padapter)
struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
struct mlme_ext_info *pmlmeinfo = &pmlmeext->mlmext_info;
- atomic_set(&pmlmeext->event_seq, 0);
+ seqnum32_set(&pmlmeext->event_seq, 0);
pmlmeext->mgnt_seq = 0;/* reset to zero when disconnect at client mode
*/
pmlmeext->sa_query_seq = 0;
pmlmeext->mgnt_80211w_IPN = 0;
@@ -5051,7 +5052,9 @@ void report_survey_event(struct adapter *padapter, union
recv_frame *precv_frame
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct survey_event);
pc2h_evt_hdr->ID = GEN_EVT_CODE(_Survey);
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
psurvey_evt = (struct survey_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
@@ -5104,7 +5107,9 @@ void report_surveydone_event(struct adapter *padapter)
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct surveydone_event);
pc2h_evt_hdr->ID = GEN_EVT_CODE(_SurveyDone);
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
psurveydone_evt = (struct surveydone_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
psurveydone_evt->bss_cnt = pmlmeext->sitesurvey_res.bss_cnt;
@@ -5151,7 +5156,9 @@ void report_join_res(struct adapter *padapter, int res)
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct joinbss_event);
pc2h_evt_hdr->ID = GEN_EVT_CODE(_JoinBss);
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
pjoinbss_evt = (struct joinbss_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
memcpy((unsigned char *)(&(pjoinbss_evt->network.network)),
&(pmlmeinfo->network), sizeof(struct wlan_bssid_ex));
@@ -5202,7 +5209,9 @@ void report_wmm_edca_update(struct adapter *padapter)
pc2h_evt_hdr = (struct C2HEvent_Header *)(pevtcmd);
pc2h_evt_hdr->len = sizeof(struct wmm_event);
pc2h_evt_hdr->ID = GEN_EVT_CODE(_WMM);
- pc2h_evt_hdr->seq = atomic_inc_return(&pmlmeext->event_seq);
+
+ seqnum32_inc(&pmlmeext->event_seq);
+ pc2h_evt_hdr->seq = seqnum32_read(&pmlmeext->event_seq);
pwmm_event = (struct wmm_event *)(pevtcmd + sizeof(struct
C2HEvent_Header));
pwmm_event->wmm = 0;
@@ -5249,7 +5258,9 @@ void report_del_sta_event(struct adapter *padapter,
unsigned char *MacAddr, u
Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
On Thu, Nov 05, 2020 at 02:44:08AM +0300, Dmitry Osipenko wrote:
> Add OPP and SoC core voltage scaling support to the display controller
> driver. This is required for enabling system-wide DVFS on older Tegra
> SoCs.
>
> Tested-by: Peter Geis
> Tested-by: Nicolas Chauvet
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/tegra/Kconfig | 1 +
> drivers/gpu/drm/tegra/dc.c| 138 +-
> drivers/gpu/drm/tegra/dc.h| 5 ++
> 3 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> index 1650a448eabd..9eec4c3fbd3b 100644
> --- a/drivers/gpu/drm/tegra/Kconfig
> +++ b/drivers/gpu/drm/tegra/Kconfig
> @@ -12,6 +12,7 @@ config DRM_TEGRA
> select INTERCONNECT
> select IOMMU_IOVA
> select CEC_CORE if CEC_NOTIFIER
> + select PM_OPP
> help
> Choose this option if you have an NVIDIA Tegra SoC.
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index fd7c8828652d..babcb66a335b 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -11,9 +11,13 @@
> #include
> #include
> #include
> +#include
> #include
> +#include
> #include
>
> +#include
> +#include
> #include
>
> #include
> @@ -1699,6 +1703,55 @@ int tegra_dc_state_setup_clock(struct tegra_dc *dc,
> return 0;
> }
>
> +static void tegra_dc_update_voltage_state(struct tegra_dc *dc,
> + struct tegra_dc_state *state)
> +{
> + struct dev_pm_opp *opp;
> + unsigned long rate;
> + int err, min_uV;
> +
> + /* OPP usage is optional */
> + if (!dc->opp_table)
> + return;
> +
> + /* calculate actual pixel clock rate which depends on internal divider
> */
> + rate = DIV_ROUND_UP(clk_get_rate(dc->clk) * 2, state->div + 2);
> +
> + /* find suitable OPP for the rate */
> + opp = dev_pm_opp_find_freq_ceil(dc->dev, &rate);
> +
> + if (opp == ERR_PTR(-ERANGE))
> + opp = dev_pm_opp_find_freq_floor(dc->dev, &rate);
> +
> + if (IS_ERR(opp)) {
> + dev_err(dc->dev, "failed to find OPP for %lu Hz: %ld\n",
> + rate, PTR_ERR(opp));
> + return;
> + }
> +
> + min_uV = dev_pm_opp_get_voltage(opp);
> + dev_pm_opp_put(opp);
> +
> + /*
> + * Voltage scaling is optional and trying to set voltage for a dummy
> + * regulator will error out.
> + */
> + if (!device_property_present(dc->dev, "core-supply"))
> + return;
This is a potentially heavy operation, so I think we should avoid that
here. How about you use devm_regulator_get_optional() in ->probe()? That
returns -ENODEV if no regulator was specified, in which case you can set
dc->core_reg = NULL and use that as the condition here.
> +
> + /*
> + * Note that the minimum core voltage depends on the pixel clock
> + * rate (which depends on internal clock divider of CRTC) and not on
> + * the rate of the display controller clock. This is why we're not
> + * using dev_pm_opp_set_rate() API and instead are managing the
> + * voltage by ourselves.
> + */
> + err = regulator_set_voltage(dc->core_reg, min_uV, INT_MAX);
> + if (err)
> + dev_err(dc->dev, "failed to set CORE voltage to %duV: %d\n",
> + min_uV, err);
> +}
Also, I'd prefer if the flow here was more linear, such as:
if (dc->core_reg) {
err = regulator_set_voltage(...);
...
}
> +
> static void tegra_dc_commit_state(struct tegra_dc *dc,
> struct tegra_dc_state *state)
> {
> @@ -1738,6 +1791,8 @@ static void tegra_dc_commit_state(struct tegra_dc *dc,
> if (err < 0)
> dev_err(dc->dev, "failed to set clock %pC to %lu Hz: %d\n",
> dc->clk, state->pclk, err);
> +
> + tegra_dc_update_voltage_state(dc, state);
> }
>
> static void tegra_dc_stop(struct tegra_dc *dc)
> @@ -2521,6 +2576,7 @@ static int tegra_dc_runtime_suspend(struct
> host1x_client *client)
>
> clk_disable_unprepare(dc->clk);
> pm_runtime_put_sync(dev);
> + regulator_disable(dc->core_reg);
>
> return 0;
> }
> @@ -2531,10 +2587,16 @@ static int tegra_dc_runtime_resume(struct
> host1x_client *client)
> struct device *dev = client->dev;
> int err;
>
> + err = regulator_enable(dc->core_reg);
> + if (err < 0) {
> + dev_err(dev, "failed to enable CORE regulator: %d\n", err);
> + return err;
> + }
> +
> err = pm_runtime_get_sync(dev);
> if (err < 0) {
> dev_err(dev, "failed to get runtime PM: %d\n", err);
> - return err;
> + goto disable_regulator;
> }
>
> if (dc->soc->has_powergate) {
> @@ -2564,6 +2626,9 @@ static int tegra_dc_runtime_resume(struct host1x_client
> *cl
Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
On Tue, Nov 10, 2020 at 09:29:45PM +0100, Thierry Reding wrote: > On Thu, Nov 05, 2020 at 02:44:08AM +0300, Dmitry Osipenko wrote: > > + /* > > +* Voltage scaling is optional and trying to set voltage for a dummy > > +* regulator will error out. > > +*/ > > + if (!device_property_present(dc->dev, "core-supply")) > > + return; > This is a potentially heavy operation, so I think we should avoid that > here. How about you use devm_regulator_get_optional() in ->probe()? That > returns -ENODEV if no regulator was specified, in which case you can set > dc->core_reg = NULL and use that as the condition here. Or enumerate the configurable voltages after getting the regulator and handle that appropriately which would be more robust in case there's missing or unusual constraints. signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
On Tue, Nov 10, 2020 at 12:53:38PM -0700, Shuah Khan wrote:
> seqnum_ops api is introduced to be used when a variable is used as
> a sequence/stat counter and doesn't guard object lifetimes. This
> clearly differentiates atomic_t usages that guard object lifetimes.
>
> seqnum32 variables wrap around to INT_MIN when it overflows and
> should not be used to guard resource lifetimes, device usage and
> open counts that control state changes, and pm states.
>
> atomic_t variables used for error_count and ios_threshold are atomic
> counters and guarded by max. values. No change to the behavior with
> this change.
>
> Signed-off-by: Shuah Khan
> ---
> .../staging/unisys/visorhba/visorhba_main.c | 37 ++-
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 7ae5306b92fe..3209958b8aaa 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
> struct visordisk_info {
> struct scsi_device *sdev;
> u32 valid;
> - atomic_t ios_threshold;
> - atomic_t error_count;
> + struct seqnum32 ios_threshold;
> + struct seqnum32 error_count;
Are you sure the threshold variable is a sequence number?
It goes up and down, not just up and up and up.
An error count just goes up :)
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 07/30] soc/tegra: Add sync state API
On Thu, Nov 05, 2020 at 02:44:04AM +0300, Dmitry Osipenko wrote:
> Introduce sync state API that will be used by Tegra device drivers. This
> new API is primarily needed for syncing state of SoC devices that are left
> ON after bootloader or permanently enabled. All these devices belong to a
> shared CORE voltage domain, and thus, we needed to bring all the devices
> into expected state before the voltage scaling could be performed.
>
> All drivers of DVFS-critical devices shall sync theirs the state before
> Tegra's voltage regulator coupler will be allowed to perform a system-wide
> voltage scaling.
>
> Tested-by: Peter Geis
> Tested-by: Nicolas Chauvet
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/soc/tegra/common.c | 152 -
> include/soc/tegra/common.h | 22 ++
> 2 files changed, 170 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index 3dc54f59cafe..f9b2b6f57887 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -3,13 +3,52 @@
> * Copyright (C) 2014 NVIDIA CORPORATION. All rights reserved.
> */
>
> +#define dev_fmt(fmt) "%s: " fmt, __func__
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include
> +#include
> +#include
> #include
> +#include
>
> #include
>
> +#define terga_soc_for_each_device(__dev) \
tegra_soc_for_each_device
> + for ((__dev) = tegra_soc_devices; (__dev) && (__dev)->compatible; \
> + (__dev)++)
> +
> +struct tegra_soc_device {
> + const char *compatible;
> + const bool dvfs_critical;
> + unsigned int sync_count;
> +};
> +
> +static DEFINE_MUTEX(tegra_soc_lock);
> +static struct tegra_soc_device *tegra_soc_devices;
> +
> +/*
> + * DVFS-critical devices are either active at a boot time or permanently
> + * active, like EMC for example. System-wide DVFS should be deferred until
> + * drivers of the critical devices synced theirs state.
> + */
> +
> +static struct tegra_soc_device tegra20_soc_devices[] = {
> + { .compatible = "nvidia,tegra20-dc", .dvfs_critical = true, },
> + { .compatible = "nvidia,tegra20-emc", .dvfs_critical = true, },
> + { }
> +};
> +
> +static struct tegra_soc_device tegra30_soc_devices[] = {
> + { .compatible = "nvidia,tegra30-dc", .dvfs_critical = true, },
> + { .compatible = "nvidia,tegra30-emc", .dvfs_critical = true, },
> + { .compatible = "nvidia,tegra30-pwm", .dvfs_critical = true, },
> + { }
> +};
> +
> static const struct of_device_id tegra_machine_match[] = {
> - { .compatible = "nvidia,tegra20", },
> - { .compatible = "nvidia,tegra30", },
> + { .compatible = "nvidia,tegra20", .data = tegra20_soc_devices, },
> + { .compatible = "nvidia,tegra30", .data = tegra30_soc_devices, },
> { .compatible = "nvidia,tegra114", },
> { .compatible = "nvidia,tegra124", },
> { .compatible = "nvidia,tegra132", },
> @@ -17,7 +56,7 @@ static const struct of_device_id tegra_machine_match[] = {
> { }
> };
>
> -bool soc_is_tegra(void)
> +static const struct of_device_id *tegra_soc_of_match(void)
> {
> const struct of_device_id *match;
> struct device_node *root;
> @@ -29,5 +68,110 @@ bool soc_is_tegra(void)
> match = of_match_node(tegra_machine_match, root);
> of_node_put(root);
>
> - return match != NULL;
> + return match;
> +}
> +
> +bool soc_is_tegra(void)
> +{
> + return tegra_soc_of_match() != NULL;
> +}
> +
> +void tegra_soc_device_sync_state(struct device *dev)
> +{
> + struct tegra_soc_device *soc_dev;
> +
> + mutex_lock(&tegra_soc_lock);
> + terga_soc_for_each_device(soc_dev) {
tegra_soc_for_each_device
> + if (!of_device_is_compatible(dev->of_node, soc_dev->compatible))
> + continue;
> +
> + if (!soc_dev->sync_count) {
> + dev_err(dev, "already synced\n");
> + break;
> + }
> +
> + /*
> + * All DVFS-capable devices should have the CORE regulator
> + * phandle. Older device-trees don't have it, hence state
> + * won't be synced for the older DTBs, allowing them to work
> + * properly.
> + */
> + if (soc_dev->dvfs_critical &&
> + !device_property_present(dev, "core-supply")) {
> + dev_dbg(dev, "doesn't have core supply\n");
> + break;
> + }
> +
> + soc_dev->sync_count--;
> + dev_dbg(dev, "sync_count=%u\n", soc_dev->sync_count);
> + break;
> + }
> + mutex_unlock(&tegra_soc_lock);
> +}
> +EXPORT_SYMBOL_GPL(tegra_soc_device_sync_state);
> +
> +bool tegra_soc_dvfs_state_synced(void)
> +{
> + struct tegra_soc_device *soc_dev;
> + bool synced_state = true;
> +
> + /*
> + * CORE voltage scaling is limited until drivers of the critical
> + * devices synced
Re: [PATCH v1 18/30] pwm: tegra: Support OPP and core voltage scaling
On Thu, Nov 05, 2020 at 02:44:15AM +0300, Dmitry Osipenko wrote:
[...]
> +static void tegra_pwm_deinit_opp_table(void *data)
> +{
> + struct device *dev = data;
> + struct opp_table *opp_table;
> +
> + opp_table = dev_pm_opp_get_opp_table(dev);
> + dev_pm_opp_of_remove_table(dev);
> + dev_pm_opp_put_regulators(opp_table);
> + dev_pm_opp_put_opp_table(opp_table);
> +}
> +
> +static int devm_tegra_pwm_init_opp_table(struct device *dev)
> +{
> + struct opp_table *opp_table;
> + const char *rname = "core";
> + int err;
> +
> + /* voltage scaling is optional */
> + if (device_property_present(dev, "core-supply"))
> + opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
> + else
> + opp_table = dev_pm_opp_get_opp_table(dev);
> +
> + if (IS_ERR(opp_table))
> + return dev_err_probe(dev, PTR_ERR(opp_table),
> + "failed to prepare OPP table\n");
> +
> + /*
> + * OPP table presence is optional and we want the set_rate() of OPP
> + * API to work similarly to clk_set_rate() if table is missing in a
> + * device-tree. The add_table() errors out if OPP is missing in DT.
> + */
> + if (device_property_present(dev, "operating-points-v2")) {
> + err = dev_pm_opp_of_add_table(dev);
> + if (err) {
> + dev_err(dev, "failed to add OPP table: %d\n", err);
> + goto put_table;
> + }
> + }
> +
> + err = devm_add_action(dev, tegra_pwm_deinit_opp_table, dev);
> + if (err)
> + goto remove_table;
> +
> + return 0;
> +
> +remove_table:
> + dev_pm_opp_of_remove_table(dev);
> +put_table:
> + dev_pm_opp_put_regulators(opp_table);
> +
> + return err;
> +}
These two functions seem to be heavily boilerplate across all these
drivers. Have you considered splitting these out into separate helpers?
Thierry
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 12/13] drivers/staging/unisys/visorhba: convert stats to use seqnum_ops
On 11/10/20 1:42 PM, Greg KH wrote:
On Tue, Nov 10, 2020 at 12:53:38PM -0700, Shuah Khan wrote:
seqnum_ops api is introduced to be used when a variable is used as
a sequence/stat counter and doesn't guard object lifetimes. This
clearly differentiates atomic_t usages that guard object lifetimes.
seqnum32 variables wrap around to INT_MIN when it overflows and
should not be used to guard resource lifetimes, device usage and
open counts that control state changes, and pm states.
atomic_t variables used for error_count and ios_threshold are atomic
counters and guarded by max. values. No change to the behavior with
this change.
Signed-off-by: Shuah Khan
---
.../staging/unisys/visorhba/visorhba_main.c | 37 ++-
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
b/drivers/staging/unisys/visorhba/visorhba_main.c
index 7ae5306b92fe..3209958b8aaa 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -41,8 +42,8 @@ MODULE_ALIAS("visorbus:" VISOR_VHBA_CHANNEL_GUID_STR);
struct visordisk_info {
struct scsi_device *sdev;
u32 valid;
- atomic_t ios_threshold;
- atomic_t error_count;
+ struct seqnum32 ios_threshold;
+ struct seqnum32 error_count;
Are you sure the threshold variable is a sequence number >
It goes up and down, not just up and up and up.
Right. I does go down. Turns out this is the only place seqnum32_dec()
is used. :)
I will fix. I noticed you made a comment about _dec() interfaces on
1/13. I can drop those as well. This way seqnum_ops can be just used
for up counters.
thanks,
-- Shuah
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
10.11.2020 23:29, Thierry Reding пишет:
>> +
>> +dc->opp_table = dev_pm_opp_get_opp_table(dc->dev);
>> +if (IS_ERR(dc->opp_table))
>> +return dev_err_probe(dc->dev, PTR_ERR(dc->opp_table),
>> + "failed to prepare OPP table\n");
>> +
>> +if (of_machine_is_compatible("nvidia,tegra20"))
>> +hw_version = BIT(tegra_sku_info.soc_process_id);
>> +else
>> +hw_version = BIT(tegra_sku_info.soc_speedo_id);
>> +
>> +hw_opp_table = dev_pm_opp_set_supported_hw(dc->dev, &hw_version, 1);
>> +err = PTR_ERR_OR_ZERO(hw_opp_table);
> What's the point of this? A more canonical version would be:
>
> if (IS_ERR(hw_opp_table)) {
> err = PTR_ERR(hw_opp_table);
> dev_err(dc->dev, ...);
> goto put_table;
> }
>
> That uses the same number of lines but is much easier to read, in my
> opinion, because it is the canonical form.
>
Your variant is much more difficult to read for me :/
I guess the only reason it could be "canonical" is because
PTR_ERR_OR_ZERO was added not so long time ago.
But don't worry, this code will be moved out in a v2 and it won't use
PTR_ERR_OR_ZERO.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 18/30] pwm: tegra: Support OPP and core voltage scaling
10.11.2020 23:50, Thierry Reding пишет:
> On Thu, Nov 05, 2020 at 02:44:15AM +0300, Dmitry Osipenko wrote:
> [...]
>> +static void tegra_pwm_deinit_opp_table(void *data)
>> +{
>> +struct device *dev = data;
>> +struct opp_table *opp_table;
>> +
>> +opp_table = dev_pm_opp_get_opp_table(dev);
>> +dev_pm_opp_of_remove_table(dev);
>> +dev_pm_opp_put_regulators(opp_table);
>> +dev_pm_opp_put_opp_table(opp_table);
>> +}
>> +
>> +static int devm_tegra_pwm_init_opp_table(struct device *dev)
>> +{
>> +struct opp_table *opp_table;
>> +const char *rname = "core";
>> +int err;
>> +
>> +/* voltage scaling is optional */
>> +if (device_property_present(dev, "core-supply"))
>> +opp_table = dev_pm_opp_set_regulators(dev, &rname, 1);
>> +else
>> +opp_table = dev_pm_opp_get_opp_table(dev);
>> +
>> +if (IS_ERR(opp_table))
>> +return dev_err_probe(dev, PTR_ERR(opp_table),
>> + "failed to prepare OPP table\n");
>> +
>> +/*
>> + * OPP table presence is optional and we want the set_rate() of OPP
>> + * API to work similarly to clk_set_rate() if table is missing in a
>> + * device-tree. The add_table() errors out if OPP is missing in DT.
>> + */
>> +if (device_property_present(dev, "operating-points-v2")) {
>> +err = dev_pm_opp_of_add_table(dev);
>> +if (err) {
>> +dev_err(dev, "failed to add OPP table: %d\n", err);
>> +goto put_table;
>> +}
>> +}
>> +
>> +err = devm_add_action(dev, tegra_pwm_deinit_opp_table, dev);
>> +if (err)
>> +goto remove_table;
>> +
>> +return 0;
>> +
>> +remove_table:
>> +dev_pm_opp_of_remove_table(dev);
>> +put_table:
>> +dev_pm_opp_put_regulators(opp_table);
>> +
>> +return err;
>> +}
>
> These two functions seem to be heavily boilerplate across all these
> drivers. Have you considered splitting these out into separate helpers?
The helper is already prepared for v2.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 07/30] soc/tegra: Add sync state API
10.11.2020 23:47, Thierry Reding пишет:
...
> tegra_soc_for_each_device
>
> I wonder if you copy/pasted this or if you got really lucky to mistype
> this all three times.
Copied of course :)
I added a special spell checking rule for this typo, but it does help
reliably.
...
>> +terga_soc_for_each_device(soc_dev) {
>> +do {
>> +/*
>> + * Devices like display controller have multiple
>> + * instances with the same compatible. Hence we need
>> + * to walk up the whole tree in order to account those
>> + * multiple instances.
>> + */
>> +np = of_find_compatible_node(prev_np, NULL,
>> + soc_dev->compatible);
>> +of_node_put(prev_np);
>> +prev_np = np;
>> +
>> +if (of_device_is_available(np)) {
>> +pr_debug("added %s\n", soc_dev->compatible);
>> +soc_dev->sync_count++;
>> +}
>> +} while (np);
>
> Maybe use for_each_compatible_node() for that inside loop?
Good point! I think there is actually an of_node_put() bug in current
variant, which for_each_compatible_node() would safe from.
>> +}
>> +
>> +return 0;
>> }
>> +postcore_initcall_sync(tegra_soc_devices_init);
>
> This is unfortunate. I recall having this discussion multiple times and
> one idea that has been floating around for a while was to let a driver
> bind against the top-level "bus" node. That has the advantage that it
> both anchors the code somewhere, so we don't have to play this game of
> checking for the SoC with soc_is_tegra(), and it properly orders this
> with respect to the child devices, so we wouldn't have to make this a
> postcore_initcall.
>
> Might be worth looking at that again, but for now this seems okay.
Thanks
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
10.11.2020 23:32, Mark Brown пишет: > On Tue, Nov 10, 2020 at 09:29:45PM +0100, Thierry Reding wrote: >> On Thu, Nov 05, 2020 at 02:44:08AM +0300, Dmitry Osipenko wrote: > >>> + /* >>> +* Voltage scaling is optional and trying to set voltage for a dummy >>> +* regulator will error out. >>> +*/ >>> + if (!device_property_present(dc->dev, "core-supply")) >>> + return; > >> This is a potentially heavy operation, so I think we should avoid that >> here. How about you use devm_regulator_get_optional() in ->probe()? That >> returns -ENODEV if no regulator was specified, in which case you can set >> dc->core_reg = NULL and use that as the condition here. > > Or enumerate the configurable voltages after getting the regulator and > handle that appropriately which would be more robust in case there's > missing or unusual constraints. > I already changed that code to use regulator_get_optional() for v2. Regarding the enumerating supported voltage.. I think this should be done by the OPP core, but regulator core doesn't work well if regulator_get() is invoked more than one time for the same device, at least there is a loud debugfs warning about an already existing directory for a regulator. It's easy to check whether the debug directory exists before creating it, like thermal framework does it for example, but then there were some other more difficult issues.. I don't recall what they were right now. Perhaps will be easier to simply get a error from regulator_set_voltage() for now because it shouldn't ever happen in practice, unless device-tree has wrong constraints. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 07/30] soc/tegra: Add sync state API
11.11.2020 00:22, Dmitry Osipenko пишет: > I added a special spell checking rule for this typo, but it does help > reliably. does *not* ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 11/30] drm/tegra: dc: Support OPP and SoC core voltage scaling
10.11.2020 23:29, Thierry Reding пишет: >> +/* legacy device-trees don't have OPP table */ >> +if (!device_property_present(dc->dev, "operating-points-v2")) >> +return 0; > "Legacy" is a bit confusing here. For one, no device trees currently > have these tables and secondly, for newer SoCs we may never need them. > I had the same thought and already improved such comments a day ago. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] media: cedrus: Add support for VP8 decoding
VP8 in Cedrus shares same engine as H264.
Note that it seems necessary to call bitstream parsing functions,
to parse frame header, otherwise decoded image is garbage. This is
contrary to what is driver supposed to do. However, values are not
really used, so this might be acceptable. It's possible that bitstream
parsing functions set some internal VPU state, which is later necessary
for proper decoding. Biggest suspect is "VP8 probs update" trigger.
Signed-off-by: Jernej Skrabec
[addressed issues from reviewer]
Signed-off-by: Emmanuel Gil Peyrot
---
Changes in v3:
- addressed comments from Ezequiel Garcia - new comments,
using new macros from VP8 UAPI, new function for waiting
on bit to be set
Changes in v2:
- rebased on top of current linux-media master branch
NOTE: This now depends on following patch:
https://patchwork.linuxtv.org/project/linux-media/patch/20201108202021.4187-1-linkma...@linkmauve.fr/
drivers/staging/media/sunxi/cedrus/Makefile | 3 +-
drivers/staging/media/sunxi/cedrus/cedrus.c | 8 +
drivers/staging/media/sunxi/cedrus/cedrus.h | 24 +
.../staging/media/sunxi/cedrus/cedrus_dec.c | 5 +
.../staging/media/sunxi/cedrus/cedrus_hw.c| 2 +
.../staging/media/sunxi/cedrus/cedrus_regs.h | 80 ++
.../staging/media/sunxi/cedrus/cedrus_video.c | 9 +
.../staging/media/sunxi/cedrus/cedrus_vp8.c | 907 ++
8 files changed, 1037 insertions(+), 1 deletion(-)
create mode 100644 drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
diff --git a/drivers/staging/media/sunxi/cedrus/Makefile
b/drivers/staging/media/sunxi/cedrus/Makefile
index 1bce49d3e7e2..a647b3690bf8 100644
--- a/drivers/staging/media/sunxi/cedrus/Makefile
+++ b/drivers/staging/media/sunxi/cedrus/Makefile
@@ -2,4 +2,5 @@
obj-$(CONFIG_VIDEO_SUNXI_CEDRUS) += sunxi-cedrus.o
sunxi-cedrus-y = cedrus.o cedrus_video.o cedrus_hw.o cedrus_dec.o \
-cedrus_mpeg2.o cedrus_h264.o cedrus_h265.o
+cedrus_mpeg2.o cedrus_h264.o cedrus_h265.o \
+cedrus_vp8.o
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
b/drivers/staging/media/sunxi/cedrus/cedrus.c
index 9a102b7c1bb9..0b8e748ef8f1 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -142,6 +142,13 @@ static const struct cedrus_control cedrus_controls[] = {
.codec = CEDRUS_CODEC_H265,
.required = false,
},
+ {
+ .cfg = {
+ .id = V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER,
+ },
+ .codec = CEDRUS_CODEC_VP8,
+ .required = true,
+ },
};
#define CEDRUS_CONTROLS_COUNT ARRAY_SIZE(cedrus_controls)
@@ -393,6 +400,7 @@ static int cedrus_probe(struct platform_device *pdev)
dev->dec_ops[CEDRUS_CODEC_MPEG2] = &cedrus_dec_ops_mpeg2;
dev->dec_ops[CEDRUS_CODEC_H264] = &cedrus_dec_ops_h264;
dev->dec_ops[CEDRUS_CODEC_H265] = &cedrus_dec_ops_h265;
+ dev->dec_ops[CEDRUS_CODEC_VP8] = &cedrus_dec_ops_vp8;
mutex_init(&dev->dev_mutex);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 93c843ae14bb..fece505272b4 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -22,6 +22,7 @@
#include
#include
+#include
#include
#define CEDRUS_NAME"cedrus"
@@ -35,6 +36,7 @@ enum cedrus_codec {
CEDRUS_CODEC_MPEG2,
CEDRUS_CODEC_H264,
CEDRUS_CODEC_H265,
+ CEDRUS_CODEC_VP8,
CEDRUS_CODEC_LAST,
};
@@ -76,6 +78,10 @@ struct cedrus_h265_run {
const struct v4l2_ctrl_hevc_slice_params*slice_params;
};
+struct cedrus_vp8_run {
+ const struct v4l2_ctrl_vp8_frame_header *frame_params;
+};
+
struct cedrus_run {
struct vb2_v4l2_buffer *src;
struct vb2_v4l2_buffer *dst;
@@ -84,6 +90,7 @@ struct cedrus_run {
struct cedrus_h264_run h264;
struct cedrus_mpeg2_run mpeg2;
struct cedrus_h265_run h265;
+ struct cedrus_vp8_run vp8;
};
};
@@ -135,6 +142,14 @@ struct cedrus_ctx {
void*neighbor_info_buf;
dma_addr_t neighbor_info_buf_addr;
} h265;
+ struct {
+ unsigned intlast_frame_p_type;
+ unsigned intlast_filter_type;
+ unsigned intlast_sharpness_level;
+
+ u8 *entropy_probs_buf;
+ dma_addr_t entropy_probs_buf_dma;
+ } vp8;
} codec;
};
@@ -181,6 +196,7 @@ struct cedrus_dev {
extern struct cedrus_dec_ops cedrus_dec_ops_mpeg2;
extern struct cedrus_dec_ops cedrus_dec_ops_h264;
extern struct cedrus_dec_ops cedrus_dec_ops_h265;
+e
Re: [RESEND PATCH 0/2] media: uapi: Expose VP8 probability lengths as defines
Hi Emmanuel, Thanks for the patch. On Mon, 9 Nov 2020 at 15:37, Emmanuel Gil Peyrot wrote: > > These values will be used by various drivers implementing the VP8 > stateless API. > > This had been suggested by Ezequiel Garcia for the Cedrus VP8 driver. > > The only driver using this API (until now) has also been updated to use > these new defines. > > This is a resend because I forgot to include most maintainers, sorry for > that. It’s my very first patch to the kernel, I didn’t know about > scripts/get_maintainers.pl > I haven't validated these two patches, but on a first look, it seems it's a low-hanging fruit nice cleanup. Thanks for that! Since it seems you are looking for interesting things to contribute, note that the vp8-ctrls.h header is lacking some nice documentation on each structure. This should be done by looking at the VP8 syntax spec and documenting things appropriately. See how it's done for H.264 and VP9: https://patchwork.linuxtv.org/project/linux-media/patch/20200928201433.327068-1-ezequ...@collabora.com/ https://patchwork.kernel.org/project/linux-rockchip/patch/20201102190551.1223389-3-adrian.ra...@collabora.com/ Thanks, Ezequiel > Emmanuel Gil Peyrot (2): > media: uapi: Expose probability lengths as defines > media: hantro: Use VP8 lengths defined in uapi > > drivers/staging/media/hantro/hantro_vp8.c | 4 ++-- > include/media/vp8-ctrls.h | 6 -- > 2 files changed, 6 insertions(+), 4 deletions(-) > > -- > 2.29.2 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
