* Re: [PATCH v26 26/33] ALSA: usb-audio: Prevent starting of audio stream if in use
From: Wesley Cheng @ 2024-09-03 23:21 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-input, linux-usb,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <e8b11cb4-cda1-4904-b83f-e124066758e3@linux.intel.com>
Hi Pierre,
On 8/30/2024 2:45 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, Wesley Cheng wrote:
>> With USB audio offloading, an audio session is started from the ASoC
>> platform sound card and PCM devices. Likewise, the USB SND path is still
>> readily available for use, in case the non-offload path is desired. In
>> order to prevent the two entities from attempting to use the USB bus,
>> introduce a flag that determines when either paths are in use.
>>
>> If a PCM device is already in use, the check will return an error to
>> userspace notifying that the stream is currently busy. This ensures that
>> only one path is using the USB substream.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> I would also move this patch earlier in the series since it has no
> dependency on USB offload really, and if somehow it breaks USB audio
> regular paths we'd want to know early for bisection.
Sure I'll re-order this earlier since I'm going to send out another rev.
Thanks
Wesley Cheng
>
>> ---
>> sound/usb/card.h | 1 +
>> sound/usb/pcm.c | 29 ++++++++++++++++++++++++++---
>> 2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>> index 15cda1730076..d8b8522e1613 100644
>> --- a/sound/usb/card.h
>> +++ b/sound/usb/card.h
>> @@ -165,6 +165,7 @@ struct snd_usb_substream {
>> unsigned int pkt_offset_adj; /* Bytes to drop from beginning of packets (for non-compliant devices) */
>> unsigned int stream_offset_adj; /* Bytes to drop from beginning of stream (for non-compliant devices) */
>>
>> + unsigned int opened:1; /* pcm device opened */
>> unsigned int running: 1; /* running status */
>> unsigned int period_elapsed_pending; /* delay period handling */
>>
>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
>> index 18467da6fd9e..b24ee38fad72 100644
>> --- a/sound/usb/pcm.c
>> +++ b/sound/usb/pcm.c
>> @@ -1241,8 +1241,17 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
>> struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
>> struct snd_pcm_runtime *runtime = substream->runtime;
>> struct snd_usb_substream *subs = &as->substream[direction];
>> + struct snd_usb_audio *chip = subs->stream->chip;
>> int ret;
>>
>> + mutex_lock(&chip->mutex);
>> + if (subs->opened) {
>> + mutex_unlock(&chip->mutex);
>> + return -EBUSY;
>> + }
>> + subs->opened = 1;
>> + mutex_unlock(&chip->mutex);
>> +
>> runtime->hw = snd_usb_hardware;
>> /* need an explicit sync to catch applptr update in low-latency mode */
>> if (direction == SNDRV_PCM_STREAM_PLAYBACK &&
>> @@ -1259,13 +1268,23 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
>>
>> ret = setup_hw_info(runtime, subs);
>> if (ret < 0)
>> - return ret;
>> + goto err_open;
>> ret = snd_usb_autoresume(subs->stream->chip);
>> if (ret < 0)
>> - return ret;
>> + goto err_open;
>> ret = snd_media_stream_init(subs, as->pcm, direction);
>> if (ret < 0)
>> - snd_usb_autosuspend(subs->stream->chip);
>> + goto err_resume;
>> +
>> + return 0;
>> +
>> +err_resume:
>> + snd_usb_autosuspend(subs->stream->chip);
>> +err_open:
>> + mutex_lock(&chip->mutex);
>> + subs->opened = 0;
>> + mutex_unlock(&chip->mutex);
>> +
>> return ret;
>> }
>>
>> @@ -1274,6 +1293,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
>> int direction = substream->stream;
>> struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
>> struct snd_usb_substream *subs = &as->substream[direction];
>> + struct snd_usb_audio *chip = subs->stream->chip;
>> int ret;
>>
>> snd_media_stop_pipeline(subs);
>> @@ -1287,6 +1307,9 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
>>
>> subs->pcm_substream = NULL;
>> snd_usb_autosuspend(subs->stream->chip);
>> + mutex_lock(&chip->mutex);
>> + subs->opened = 0;
>> + mutex_unlock(&chip->mutex);
>>
>> return 0;
>> }
^ permalink raw reply
* Re: [PATCH v26 24/33] ALSA: usb-audio: Introduce USB SND platform op callbacks
From: Wesley Cheng @ 2024-09-03 23:20 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-input, linux-usb,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <63b679c8-48f1-4251-8b7e-d38b605e5089@linux.intel.com>
Hi Pierre,
On 8/30/2024 2:38 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, Wesley Cheng wrote:
>> Allow for different platforms to be notified on USB SND connect/disconnect
>> sequences. This allows for platform USB SND modules to properly initialize
>> and populate internal structures with references to the USB SND chip
>> device.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>> sound/usb/card.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>> sound/usb/card.h | 10 +++++++++
>> 2 files changed, 63 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 1f9dfcd8f336..7f120aa006c0 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -118,6 +118,42 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no)
>> static DEFINE_MUTEX(register_mutex);
>> static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
>> static struct usb_driver usb_audio_driver;
>> +static struct snd_usb_platform_ops *platform_ops;
>> +
>> +/*
>> + * Register platform specific operations that will be notified on events
>> + * which occur in USB SND. The platform driver can utilize this path to
>> + * enable features, such as USB audio offloading, which allows for audio data
>> + * to be queued by an audio DSP.
>> + *
>> + * Only one set of platform operations can be registered to USB SND. The
>> + * platform register operation is protected by the register_mutex.
>> + */
>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>> +{
>> + guard(mutex)(®ister_mutex);
>> + if (platform_ops)
>> + return -EEXIST;
>> +
>> + platform_ops = ops;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
>> +
>> +/*
>> + * Unregisters the current set of platform operations. This allows for
> Unregister?
Will fix.
>> + * a new set to be registered if required.
>> + *
>> + * The platform unregister operation is protected by the register_mutex.
>> + */
>> +int snd_usb_unregister_platform_ops(void)
>> +{
>> + guard(mutex)(®ister_mutex);
>> + platform_ops = NULL;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
>>
>> /*
>> * Checks to see if requested audio profile, i.e sample rate, # of
>> @@ -946,7 +982,11 @@ static int usb_audio_probe(struct usb_interface *intf,
>> chip->num_interfaces++;
>> usb_set_intfdata(intf, chip);
>> atomic_dec(&chip->active);
>> +
>> + if (platform_ops && platform_ops->connect_cb)
>> + platform_ops->connect_cb(chip);
>> mutex_unlock(®ister_mutex);
>> +
>> return 0;
>>
>> __error:
>> @@ -983,6 +1023,9 @@ static void usb_audio_disconnect(struct usb_interface *intf)
>> card = chip->card;
>>
>> mutex_lock(®ister_mutex);
>> + if (platform_ops && platform_ops->disconnect_cb)
>> + platform_ops->disconnect_cb(chip);
>> +
>> if (atomic_inc_return(&chip->shutdown) == 1) {
>> struct snd_usb_stream *as;
>> struct snd_usb_endpoint *ep;
>> @@ -1130,6 +1173,11 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
>> chip->system_suspend = chip->num_suspended_intf;
>> }
>>
>> + mutex_lock(®ister_mutex);
>> + if (platform_ops && platform_ops->suspend_cb)
>> + platform_ops->suspend_cb(intf, message);
>> + mutex_unlock(®ister_mutex);
>> +
>> return 0;
>> }
>>
>> @@ -1170,6 +1218,11 @@ static int usb_audio_resume(struct usb_interface *intf)
>>
>> snd_usb_midi_v2_resume_all(chip);
>>
>> + mutex_lock(®ister_mutex);
>> + if (platform_ops && platform_ops->resume_cb)
>> + platform_ops->resume_cb(intf);
>> + mutex_unlock(®ister_mutex);
>> +
>> out:
>> if (chip->num_suspended_intf == chip->system_suspend) {
>> snd_power_change_state(chip->card, SNDRV_CTL_POWER_D0);
>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>> index 4f4f3f39b7fa..23d9e6fc69e7 100644
>> --- a/sound/usb/card.h
>> +++ b/sound/usb/card.h
>> @@ -207,7 +207,17 @@ struct snd_usb_stream {
>> struct list_head list;
>> };
>>
>> +struct snd_usb_platform_ops {
>> + void (*connect_cb)(struct snd_usb_audio *chip);
>> + void (*disconnect_cb)(struct snd_usb_audio *chip);
>> + void (*suspend_cb)(struct usb_interface *intf, pm_message_t message);
>> + void (*resume_cb)(struct usb_interface *intf);
>> +};
>
> You're using the same mutex to protect all four callbacks, so how would
> things work if e.g. you disconnected a device during the resume operation?
We actually might be able to remove the mutex locks from the suspend and resume callbacks. From looking at the USB core driver, whenever the USB interface is unbounded, it ensures that it is in an active/resumed state before the disconnect() is called:
static int usb_unbind_interface(struct device *dev)
{
..
/* Autoresume for set_interface call below */
udev = interface_to_usbdev(intf);
error = usb_autoresume_device(udev);
..
driver->disconnect(intf);
So this will ensure that there won't be a condition where an interface disconnect routine could run in parallel to the interface's runtime resume or runtime suspend.
Thanks
Wesley Cheng
^ permalink raw reply
* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
From: Dmitry Torokhov @ 2024-09-03 22:03 UTC (permalink / raw)
To: Richard Acayan
Cc: Marge Yang, linux-input, linux-kernel, Vincent Huang, david.chiu,
derek.cheng, sam.tsai
In-Reply-To: <ZteAo-bklYbs29Pq@radian>
On Tue, Sep 03, 2024 at 05:33:23PM -0400, Richard Acayan wrote:
> On Tue, Sep 03, 2024 at 11:40:38AM -0700, Dmitry Torokhov wrote:
> > On Tue, Sep 03, 2024 at 02:07:23PM -0400, Richard Acayan wrote:
> > > > + /* Use the Query DPM feature when the query register exists for resolution. */
> > > > + item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
> > > > + if (item) {
> > > > + offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> > > > + RMI_F12_QUERY_RESOLUTION);
> > > > + query_dpm_addr = fn->fd.query_base_addr + offset;
> > > > + ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
> > > > + if (ret < 0) {
> > > > + dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
> > > > + return -ENODEV;
> > > > + }
> > > > + dpm_resolution = buf[0];
> > > > +
> > > > + sensor->x_mm = sensor->max_x / dpm_resolution;
> > > > + sensor->y_mm = sensor->max_y / dpm_resolution;
> > > > + } else {
> > > > + if (rmi_register_desc_has_subpacket(item, 3)) {
> > >
> > > The item variable is NULL in this branch, as it was overwritten just
> > > before the if statement.
> > >
> > > This patch causes a NULL pointer dereference:
> >
> > Ugh, indeed. I guess the simplest way of fixing this would be:
> >
> > diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> > index fc2cc8e2b0ba..8246fe77114b 100644
> > --- a/drivers/input/rmi4/rmi_f12.c
> > +++ b/drivers/input/rmi4/rmi_f12.c
> > @@ -129,9 +129,8 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
> > * Use the Query DPM feature when the resolution query register
> > * exists.
> > */
> > - item = rmi_get_register_desc_item(&f12->query_reg_desc,
> > - RMI_F12_QUERY_RESOLUTION);
> > - if (item) {
> > + if (rmi_get_register_desc_item(&f12->query_reg_desc,
> > + RMI_F12_QUERY_RESOLUTION)) {
> > offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> > RMI_F12_QUERY_RESOLUTION);
> > query_dpm_addr = fn->fd.query_base_addr + offset;
> >
> > Could you please tell me if this works for you?
>
> Yeah, it fixes the bug.
Great, thank you for reporting and testing!
--
Dmitry
^ permalink raw reply
* Re: [PATCH v26 23/33] ASoC: qcom: qdsp6: Fetch USB offload mapped card and PCM device
From: Wesley Cheng @ 2024-09-03 21:49 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-input, linux-usb,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <87b06b92-8e58-414d-ba53-db7c88ac525a@linux.intel.com>
Hi Pierre,
On 8/30/2024 2:34 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, Wesley Cheng wrote:
>> The USB SND path may need to know how the USB offload path is routed, so
>> that applications can open the proper sound card and PCM device. The
>> implementation for the QC ASoC design has a "USB Mixer" kcontrol for each
>> possible FE (Q6ASM) DAI, which can be utilized to know which front end link
>> is enabled.
>>
>> When an application/userspace queries for the mapped offload devices, the
>> logic will lookup the USB mixer status though the following path:
>>
>> MultiMedia* <-> MM_DL* <-> USB Mixer*
>>
>> The "USB Mixer" is a DAPM widget, and the q6routing entity will set the
>> DAPM connect status accordingly if the USB mixer is enabled. If enabled,
>> the Q6USB backend link can fetch the PCM device number from the FE DAI
>> link (Multimedia*). With respects to the card number, that is
>> straightforward, as the ASoC components have direct references to the ASoC
>> platform sound card.
>>
>> An example output can be shown below:
>>
>> Number of controls: 9
>> name value
>> Capture Channel Map 0, 0 (range 0->36)
>> Playback Channel Map 0, 0 (range 0->36)
>> Headset Capture Switch On
>> Headset Capture Volume 1 (range 0->4)
>> Sidetone Playback Switch On
>> Sidetone Playback Volume 4096 (range 0->8192)
>> Headset Playback Switch On
>> Headset Playback Volume 20, 20 (range 0->24)
>> USB Offload Playback Route PCM#0 0, 1 (range -1->255)
>>
>> The "USB Offload Playback Route PCM#*" kcontrol will signify the
>> corresponding card and pcm device it is offload to. (card#0 pcm - device#1)
>> If the USB SND device supports multiple audio interfaces, then it will
>> contain several PCM streams, hence in those situations, it is expected
>> that there will be multiple playback route kcontrols created.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>> sound/soc/qcom/qdsp6/q6usb.c | 104 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 104 insertions(+)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c
>> index 10337d70eb27..c2fc0dedf430 100644
>> --- a/sound/soc/qcom/qdsp6/q6usb.c
>> +++ b/sound/soc/qcom/qdsp6/q6usb.c
>> @@ -132,6 +132,109 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone
>> return ret;
>> }
>>
>> +static int q6usb_get_pcm_id_from_widget(struct snd_soc_dapm_widget *w)
>> +{
>> + struct snd_soc_pcm_runtime *rtd;
>> + struct snd_soc_dai *dai;
>> +
>> + for_each_card_rtds(w->dapm->card, rtd) {
>> + dai = snd_soc_rtd_to_cpu(rtd, 0);
>> + /*
>> + * Only look for playback widget. RTD number carries the assigned
>> + * PCM index.
>> + */
>> + if (dai->stream[0].widget == w)
>> + return rtd->num;
>> + }
>> +
>> + return -1;
>> +}
>> +
>> +static int q6usb_usb_mixer_enabled(struct snd_soc_dapm_widget *w)
>> +{
>> + struct snd_soc_dapm_path *p;
>> +
>> + /* Checks to ensure USB path is enabled/connected */
>> + snd_soc_dapm_widget_for_each_sink_path(w, p)
>> + if (!strcmp(p->sink->name, "USB Mixer") && p->connect)
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> +static int q6usb_get_pcm_id(struct snd_soc_component *component)
>> +{
>> + struct snd_soc_dapm_widget *w;
>> + struct snd_soc_dapm_path *p;
>> + int pidx;
>> +
>> + /*
>> + * Traverse widgets to find corresponding FE widget. The DAI links are
>> + * built like the following:
>> + * MultiMedia* <-> MM_DL* <-> USB Mixer*
>> + */
>> + for_each_card_widgets(component->card, w) {
>> + if (!strncmp(w->name, "MultiMedia", 10)) {
>> + /*
>> + * Look up all paths associated with the FE widget to see if
>> + * the USB BE is enabled. The sink widget is responsible to
>> + * link with the USB mixers.
>> + */
>> + snd_soc_dapm_widget_for_each_sink_path(w, p) {
>> + if (q6usb_usb_mixer_enabled(p->sink)) {
>> + pidx = q6usb_get_pcm_id_from_widget(w);
>> + return pidx;
>> + }
>> + }
> Humm, there should be a note that the design assumes that the USB
> offload path exposes a single PCM per endpoints - same as the
> non-offloaded path. If the ASoC card has multiple PCMs for each
> endpoint, possibly with different processing on each PCM, then the
> mapping would fail.
Sure I'll add a note within the comments on the above.
> The other question is whether you need to walk in the DAPM graph, in
> theory DPCM has helpers to find which FEs are connected to which BE.
Hmm...Yes, I've tried to see if I could utilize some of the helpers that were present, but none of them was able to fetch the DAPM widget that was associated with the FE, hence why I had to implement the lookup that would work for our current design.
Thanks
Wesley Cheng
^ permalink raw reply
* Re: [PATCH v26 22/33] ASoC: qcom: qdsp6: Add headphone jack for offload connection status
From: Wesley Cheng @ 2024-09-03 21:41 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-input, linux-usb,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <39e1e90e-116c-4f13-b223-84e6991c8a32@linux.intel.com>
Hi Pierre,
On 8/30/2024 2:27 AM, Pierre-Louis Bossart wrote:
>
>> /* Selects the latest USB headset plugged in for offloading */
>> + if (data->hs_jack && list_empty(&data->devices))
>> + snd_jack_report(data->hs_jack->jack, SND_JACK_USB);
>> +
> with the list_empty check, this looks like only the first connected
> headset will be handled, not the last?
Sorry, the comment is misplaced. It should be meant to explain:
/* Selects the latest USB headset plugged in for offloading */
list_add_tail(&sdev->list, &data->devices);
The above IF check is to say that we'll only notify the USB jack if there is an available USB audio device (capable of offloading) connected. I guess it might make sense to notify the snd jack on every USB audio device connection. Currently, it will notify on the first device identified (present) and the last device removed (not present).
Thanks
Wesley Cheng
>> list_add_tail(&sdev->list, &data->devices);
>> } else {
>> list_del(&sdev->list);
>> +
>> + if (data->hs_jack && list_empty(&data->devices))
>> + snd_jack_report(data->hs_jack->jack, 0);
>> }
>> mutex_unlock(&data->mutex);
>>
>> return 0;
>> }
^ permalink raw reply
* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
From: Richard Acayan @ 2024-09-03 21:33 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Marge Yang, linux-input, linux-kernel, Vincent Huang, david.chiu,
derek.cheng, sam.tsai
In-Reply-To: <ZtdYJkU17y1iNsLG@google.com>
On Tue, Sep 03, 2024 at 11:40:38AM -0700, Dmitry Torokhov wrote:
> On Tue, Sep 03, 2024 at 02:07:23PM -0400, Richard Acayan wrote:
> > > + /* Use the Query DPM feature when the query register exists for resolution. */
> > > + item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
> > > + if (item) {
> > > + offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> > > + RMI_F12_QUERY_RESOLUTION);
> > > + query_dpm_addr = fn->fd.query_base_addr + offset;
> > > + ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
> > > + if (ret < 0) {
> > > + dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
> > > + return -ENODEV;
> > > + }
> > > + dpm_resolution = buf[0];
> > > +
> > > + sensor->x_mm = sensor->max_x / dpm_resolution;
> > > + sensor->y_mm = sensor->max_y / dpm_resolution;
> > > + } else {
> > > + if (rmi_register_desc_has_subpacket(item, 3)) {
> >
> > The item variable is NULL in this branch, as it was overwritten just
> > before the if statement.
> >
> > This patch causes a NULL pointer dereference:
>
> Ugh, indeed. I guess the simplest way of fixing this would be:
>
> diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> index fc2cc8e2b0ba..8246fe77114b 100644
> --- a/drivers/input/rmi4/rmi_f12.c
> +++ b/drivers/input/rmi4/rmi_f12.c
> @@ -129,9 +129,8 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
> * Use the Query DPM feature when the resolution query register
> * exists.
> */
> - item = rmi_get_register_desc_item(&f12->query_reg_desc,
> - RMI_F12_QUERY_RESOLUTION);
> - if (item) {
> + if (rmi_get_register_desc_item(&f12->query_reg_desc,
> + RMI_F12_QUERY_RESOLUTION)) {
> offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> RMI_F12_QUERY_RESOLUTION);
> query_dpm_addr = fn->fd.query_base_addr + offset;
>
> Could you please tell me if this works for you?
Yeah, it fixes the bug.
^ permalink raw reply
* Re: [PATCH v26 21/33] ASoC: qcom: qdsp6: Add USB backend ASoC driver for Q6
From: Wesley Cheng @ 2024-09-03 21:18 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-input, linux-usb,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <afe37014-8ec5-4808-bc24-09ac0f2d93b6@linux.intel.com>
Hi Pierre,
On 8/30/2024 2:21 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, Wesley Cheng wrote:
>> Create a USB BE component that will register a new USB port to the ASoC USB
>> framework. This will handle determination on if the requested audio
>> profile is supported by the USB device currently selected.
>>
>> Check for if the PCM format is supported during the hw_params callback. If
>> the profile is not supported then the userspace ALSA entity will receive an
>> error, and can take further action.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>> include/sound/q6usboffload.h | 20 +++
>> sound/soc/qcom/Kconfig | 10 ++
>> sound/soc/qcom/qdsp6/Makefile | 1 +
>> sound/soc/qcom/qdsp6/q6usb.c | 246 ++++++++++++++++++++++++++++++++++
>> 4 files changed, 277 insertions(+)
>> create mode 100644 include/sound/q6usboffload.h
>> create mode 100644 sound/soc/qcom/qdsp6/q6usb.c
>>
>> diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h
>> new file mode 100644
>> index 000000000000..49ab2c34b84c
>> --- /dev/null
>> +++ b/include/sound/q6usboffload.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * linux/sound/q6usboffload.h -- QDSP6 USB offload
> not sure about the linux/ prefix?
>
>> + *
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +/**
>> + * struct q6usb_offload
>> + * @dev - dev handle to usb be
>> + * @sid - streamID for iommu
>> + * @intr_num - usb interrupter number
>> + * @domain - allocated iommu domain
>> + **/
>> +struct q6usb_offload {
>> + struct device *dev;
>> + long long sid;
>> + u16 intr_num;
>> + struct iommu_domain *domain;
>> +};
> consider reordering to avoid holes/alignment issues, e.g. all pointers
> first, then long long then u16
>
Will fix these.
>> +static int q6usb_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct q6usb_port_data *data = dev_get_drvdata(dai->dev);
>> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
>> + int direction = substream->stream;
>> + struct q6afe_port *q6usb_afe;
>> + struct snd_soc_usb_device *sdev;
>> + int ret = -EINVAL;
>> +
>> + mutex_lock(&data->mutex);
>> +
>> + /* No active chip index */
>> + if (list_empty(&data->devices))
>> + goto out;
> -ENODEV for the default return value>?
Sure.
>> +
>> + sdev = list_last_entry(&data->devices, struct snd_soc_usb_device, list);
>> +
>> + ret = snd_soc_usb_find_supported_format(sdev->chip_idx, params, direction);
>> + if (ret < 0)
>> + goto out;
>> +
>> + q6usb_afe = q6afe_port_get_from_id(cpu_dai->dev, USB_RX);
>> + if (IS_ERR(q6usb_afe))
>> + goto out;
>> +
>> + /* Notify audio DSP about the devices being offloaded */
>> + ret = afe_port_send_usb_dev_param(q6usb_afe, sdev->card_idx,
>> + sdev->ppcm_idx[sdev->num_playback - 1]);
> don't you need a symmetrical notification upon hw_free()?
>
> Also what happens if there are multiple calls to hw_params, which is
> quite legit in ALSA/ASoC?
The afe_port_send_usb_dev_param() is meant to just update the device selected for offload on the audio DSP end, and this won't be referenced until our Q6AFE DAI sends the port start command in its prepare() callback. Don't think we need to handle anything specific in the hw_free() case. As long as the hw_params() callback is called before any audio session is started, then we'll ensure that the device selected is always updated to the audio DSP.
Thanks
Wesley Cheng
^ permalink raw reply
* Re: [PATCH v1 10/22] iio: dac: max517: Get platform data via dev_get_platdata()
From: Andy Shevchenko @ 2024-09-03 20:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, David Lechner, Michael Hennerich,
Antoniu Miclaus, Jinjie Ruan, Lorenzo Bianconi,
Srinivas Pandruvada, Basavaraj Natikar, linux-input, linux-iio,
linux-kernel, Jiri Kosina, Lars-Peter Clausen
In-Reply-To: <20240903204737.710e49dd@jic23-huawei>
On Tue, Sep 03, 2024 at 08:47:37PM +0100, Jonathan Cameron wrote:
> On Tue, 3 Sep 2024 01:16:55 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
...
> > * Reference voltage on MAX518 and default is 5V, else take vref_mv
> > - * from platform_data
> > + * from platform_data.
>
> I guess this is accidental?
It can be dropped.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v1 10/22] iio: dac: max517: Get platform data via dev_get_platdata()
From: Jonathan Cameron @ 2024-09-03 19:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, David Lechner, Andy Shevchenko,
Michael Hennerich, Antoniu Miclaus, Jinjie Ruan, Lorenzo Bianconi,
Srinivas Pandruvada, Basavaraj Natikar, linux-input, linux-iio,
linux-kernel, Jiri Kosina, Lars-Peter Clausen
In-Reply-To: <20240902222824.1145571-11-andy.shevchenko@gmail.com>
On Tue, 3 Sep 2024 01:16:55 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Access to platform data via dev_get_platdata() getter to make code cleaner.
>
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> drivers/iio/dac/max517.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/dac/max517.c b/drivers/iio/dac/max517.c
> index 685980184d3c..96781ae04f9d 100644
> --- a/drivers/iio/dac/max517.c
> +++ b/drivers/iio/dac/max517.c
> @@ -143,10 +143,10 @@ static const struct iio_chan_spec max517_channels[] = {
>
> static int max517_probe(struct i2c_client *client)
> {
> + const struct max517_platform_data *platform_data = dev_get_platdata(&client->dev);
> const struct i2c_device_id *id = i2c_client_get_device_id(client);
> struct max517_data *data;
> struct iio_dev *indio_dev;
> - struct max517_platform_data *platform_data = client->dev.platform_data;
> int chan;
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> @@ -176,7 +176,7 @@ static int max517_probe(struct i2c_client *client)
>
> /*
> * Reference voltage on MAX518 and default is 5V, else take vref_mv
> - * from platform_data
> + * from platform_data.
I guess this is accidental?
J
> */
> for (chan = 0; chan < indio_dev->num_channels; chan++) {
> if (id->driver_data == ID_MAX518 || !platform_data)
^ permalink raw reply
* Re: [PATCH v2 2/3] input: touchscreen: ad7877: add dt support
From: Dmitry Torokhov @ 2024-09-03 18:50 UTC (permalink / raw)
To: Antoniu Miclaus
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Hennerich,
linux-input, devicetree, linux-kernel
In-Reply-To: <20240902082707.4325-2-antoniu.miclaus@analog.com>
Hi Antoniu,
On Mon, Sep 02, 2024 at 11:24:32AM +0300, Antoniu Miclaus wrote:
> Add devicetree support within the driver.
>
> Make the old platform data approach optional.
Nobody is using it, so simply remove it.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> new in v2.
> drivers/input/touchscreen/ad7877.c | 68 +++++++++++++++++++++++++++++-
> 1 file changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
> index 7886454a19c6..3fa38043b561 100644
> --- a/drivers/input/touchscreen/ad7877.c
> +++ b/drivers/input/touchscreen/ad7877.c
> @@ -27,6 +27,7 @@
> #include <linux/input.h>
> #include <linux/interrupt.h>
> #include <linux/pm.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
> #include <linux/spi/spi.h>
> #include <linux/spi/ad7877.h>
> @@ -667,6 +668,68 @@ static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct ad7877 *ts)
> }
> }
>
> +static struct ad7877_platform_data *ad7877_parse_props(struct device *dev)
> +{
> + struct ad7877_platform_data *pdata;
> + u32 value, average;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + pdata->model = (uintptr_t)device_get_match_data(dev);
> +
> + device_property_read_u8(dev, "adi,stopacq-polarity",
> + &pdata->stopacq_polarity);
> + device_property_read_u8(dev, "adi,first-conv-delay",
> + &pdata->first_conversion_delay);
> + device_property_read_u8(dev, "adi,pen-down-acc-interval",
> + &pdata->pen_down_acc_interval);
> + device_property_read_u8(dev, "adi,acquisition-time",
> + &pdata->acquisition_time);
> +
> + device_property_read_u16(dev, "adi,vref-delay-usecs",
> + &pdata->vref_delay_usecs);
> +
> + device_property_read_u32(dev, "touchscreen-x-plate-ohms", &value);
> + pdata->x_plate_ohms = (u16)value;
> + device_property_read_u32(dev, "touchscreen-y-plate-ohms", &value);
> + pdata->y_plate_ohms = (u16)value;
> + device_property_read_u32(dev, "touchscreen-min-x", &value);
> + pdata->x_min = (u16)value;
> + device_property_read_u32(dev, "touchscreen-min-y", &value);
> + pdata->y_min = (u16)value;
> + device_property_read_u32(dev, "touchscreen-max-x", &value);
> + pdata->x_max = (u16)value;
> + device_property_read_u32(dev, "touchscreen-max-y", &value);
> + pdata->y_max = (u16)value;
> + device_property_read_u32(dev, "touchscreen-max-pressure", &value);
> + pdata->pressure_max = (u16)value;
> + device_property_read_u32(dev, "touchscreen-min-pressure", &value);
> + pdata->pressure_min = (u16)value;
Please use touchscreen_parse_properties() and also apply transformations
via touchscreen_report_pos() instead of rolling your own logic.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
From: Dmitry Torokhov @ 2024-09-03 18:40 UTC (permalink / raw)
To: Richard Acayan
Cc: Marge Yang, linux-input, linux-kernel, Vincent Huang, david.chiu,
derek.cheng, sam.tsai
In-Reply-To: <ZtdQW7nqAOEJDNBN@radian>
On Tue, Sep 03, 2024 at 02:07:23PM -0400, Richard Acayan wrote:
> > + /* Use the Query DPM feature when the query register exists for resolution. */
> > + item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
> > + if (item) {
> > + offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> > + RMI_F12_QUERY_RESOLUTION);
> > + query_dpm_addr = fn->fd.query_base_addr + offset;
> > + ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
> > + if (ret < 0) {
> > + dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
> > + return -ENODEV;
> > + }
> > + dpm_resolution = buf[0];
> > +
> > + sensor->x_mm = sensor->max_x / dpm_resolution;
> > + sensor->y_mm = sensor->max_y / dpm_resolution;
> > + } else {
> > + if (rmi_register_desc_has_subpacket(item, 3)) {
>
> The item variable is NULL in this branch, as it was overwritten just
> before the if statement.
>
> This patch causes a NULL pointer dereference:
Ugh, indeed. I guess the simplest way of fixing this would be:
diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index fc2cc8e2b0ba..8246fe77114b 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -129,9 +129,8 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
* Use the Query DPM feature when the resolution query register
* exists.
*/
- item = rmi_get_register_desc_item(&f12->query_reg_desc,
- RMI_F12_QUERY_RESOLUTION);
- if (item) {
+ if (rmi_get_register_desc_item(&f12->query_reg_desc,
+ RMI_F12_QUERY_RESOLUTION)) {
offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
RMI_F12_QUERY_RESOLUTION);
query_dpm_addr = fn->fd.query_base_addr + offset;
Could you please tell me if this works for you?
Thanks.
--
Dmitry
^ permalink raw reply related
* Re: [RFC PATCH] HID: wacom: Stop mangling tool IDs
From: Dmitry Torokhov @ 2024-09-03 18:32 UTC (permalink / raw)
To: Gerecke, Jason
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Ping Cheng,
Tobita, Tatsunosuke, Erin Skomra, Joshua Dickens, Peter Hutterer,
Jason Gerecke
In-Reply-To: <20240903182633.870892-1-jason.gerecke@wacom.com>
On Tue, Sep 03, 2024 at 11:26:33AM -0700, Gerecke, Jason wrote:
> From: Jason Gerecke <jason.gerecke@wacom.com>
>
> In ancient times, an off-by-one-nybble error resulted in the Wacom
> driver sending "mangled" tool IDs to userspace. This mangling behavior
> was later enshrined into a function so that devices using the then-new
> generic codepath could share the same broken behavior. The mangled IDs
> have not historically been a major problem for userspace since few
> applications care about the exact numeric value of the IDs. As long as
> the IDs were unique it didn't much matter. Some programs (cross-
> platform and remote-desktop applications in particular) /do/ rely on
> the IDs being correct, however.
>
> This patch rids the driver of the mangling behavior.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> References: 493630b20389 ("Input: wacom - fix serial number handling on Cintiq 21UX2")
> References: 82527da319ee ("HID: wacom: Read and internally use corrected Intuos tool IDs")
> ---
> I'd like to get the opinion of the kernel maintainers on making a
> change like this at some point in the future. There are _very_ few
> userspace uses of these IDs (primarily: drivers, compositors, and
> tablet control panels) and my plan is to update those bits and then
> give some time for the changes to roll out to users before re-
> submitting this for real. I don't expect any kind of breakage since
> we'll be taking our time with the rollout and userspace needs to
> have handling for "unknown" IDs anyway (since Wacom periodically
> releases new pens).
I think if you take care of users of this data (it is Wacom-specific
anyway because ABS_MISC does not have a defined behavior) I'd be fine
changing the kernel.
Up to Jiri/Benjamin though.
Thanks.
--
Dmitry
^ permalink raw reply
* [RFC PATCH] HID: wacom: Stop mangling tool IDs
From: Gerecke, Jason @ 2024-09-03 18:26 UTC (permalink / raw)
To: linux-input, Benjamin Tissoires, Jiri Kosina, Dmitry Torokhov
Cc: Ping Cheng, Tobita, Tatsunosuke, Erin Skomra, Joshua Dickens,
Peter Hutterer, Jason Gerecke
From: Jason Gerecke <jason.gerecke@wacom.com>
In ancient times, an off-by-one-nybble error resulted in the Wacom
driver sending "mangled" tool IDs to userspace. This mangling behavior
was later enshrined into a function so that devices using the then-new
generic codepath could share the same broken behavior. The mangled IDs
have not historically been a major problem for userspace since few
applications care about the exact numeric value of the IDs. As long as
the IDs were unique it didn't much matter. Some programs (cross-
platform and remote-desktop applications in particular) /do/ rely on
the IDs being correct, however.
This patch rids the driver of the mangling behavior.
Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
References: 493630b20389 ("Input: wacom - fix serial number handling on Cintiq 21UX2")
References: 82527da319ee ("HID: wacom: Read and internally use corrected Intuos tool IDs")
---
I'd like to get the opinion of the kernel maintainers on making a
change like this at some point in the future. There are _very_ few
userspace uses of these IDs (primarily: drivers, compositors, and
tablet control panels) and my plan is to update those bits and then
give some time for the changes to roll out to users before re-
submitting this for real. I don't expect any kind of breakage since
we'll be taking our time with the rollout and userspace needs to
have handling for "unknown" IDs anyway (since Wacom periodically
releases new pens).
---
drivers/hid/wacom_wac.c | 18 ++----------------
1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 2541fa2e0fa3b..facab5bd37e41 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -671,11 +671,6 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
return 1;
}
-static int wacom_intuos_id_mangle(int tool_id)
-{
- return (tool_id & ~0xFFF) << 4 | (tool_id & 0xFFF);
-}
-
static bool wacom_is_art_pen(int tool_id)
{
bool is_art_pen = false;
@@ -1010,8 +1005,7 @@ static int wacom_intuos_general(struct wacom_wac *wacom)
break;
}
- input_report_abs(input, ABS_MISC,
- wacom_intuos_id_mangle(wacom->id[idx])); /* report tool id */
+ input_report_abs(input, ABS_MISC, wacom->id[idx]); /* report tool id */
input_report_key(input, wacom->tool[idx], 1);
input_event(input, EV_MSC, MSC_SERIAL, wacom->serial[idx]);
wacom->reporting_data = true;
@@ -1379,8 +1373,7 @@ static void wacom_intuos_pro2_bt_pen(struct wacom_wac *wacom)
input_report_key(pen_input, wacom->tool[0], prox);
input_event(pen_input, EV_MSC, MSC_SERIAL, wacom->serial[0]);
- input_report_abs(pen_input, ABS_MISC,
- wacom_intuos_id_mangle(wacom->id[0])); /* report tool id */
+ input_report_abs(pen_input, ABS_MISC, wacom->id[0]); /* report tool id */
}
wacom->shared->stylus_in_proximity = prox;
@@ -2514,13 +2507,6 @@ static void wacom_wac_pen_report(struct hid_device *hdev,
input_report_key(input, BTN_STYLUS2, wacom_wac->hid_data.barrelswitch2);
input_report_key(input, BTN_STYLUS3, wacom_wac->hid_data.barrelswitch3);
- /*
- * Non-USI EMR tools should have their IDs mangled to
- * match the legacy behavior of wacom_intuos_general
- */
- if (wacom_wac->serial[0] >> 52 == 1)
- id = wacom_intuos_id_mangle(id);
-
/*
* To ensure compatibility with xf86-input-wacom, we should
* report the BTN_TOOL_* event prior to the ABS_MISC or
--
2.46.0
^ permalink raw reply related
* Re: [PATCH v2 2/3] iio: imu: st_lsm6dsx: Use aligned data type for timestamp
From: Andy Shevchenko @ 2024-09-03 18:09 UTC (permalink / raw)
To: Uwe Kleine-König, Jonathan Cameron, Srinivas Pandruvada,
Basavaraj Natikar, linux-input, linux-iio, linux-kernel
Cc: Jiri Kosina, Jonathan Cameron, Lars-Peter Clausen,
Lorenzo Bianconi
In-Reply-To: <20240903180218.3640501-3-andriy.shevchenko@linux.intel.com>
On Tue, Sep 03, 2024 at 08:59:05PM +0300, Andy Shevchenko wrote:
> Use __aligned_s64 for the timestamp field.
Oh, heck. I forgot to change the commit message...
...
> - /* Ensure natural alignment of buffer elements */
...and for some reason this haven't been updated to be not removed.
I have updated locally, but will wait for other comments, maybe it's the only
problem and Jonathan can fix whilst applying.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
From: Richard Acayan @ 2024-09-03 18:07 UTC (permalink / raw)
To: Marge Yang, Dmitry Torokhov, linux-input, linux-kernel,
Vincent Huang
Cc: david.chiu, derek.cheng, sam.tsai
In-Reply-To: <20240805083636.1381205-1-marge.yang@tw.synaptics.com>
Hi,
On Mon, Aug 05, 2024 at 08:36:36AM +0000, Marge Yang wrote:
> Newer firmware allows to query touchpad resolution
> information by reading from resolution register.
> Presence of resolution register is signalled via bit
> 29 of the "register presence" register.
> On devices that lack this resolution register
> we fall back to using pitch and number of receivers
> data to calculate size of the sensor.
>
> RMI4 F12 will support to query DPM value on Touchpad.
> When TP firmware doesn't support to report logical and
> physical value within the Touchpad's HID report.
> We can directly query the DPM value through RMI.
>
> Signed-off-by: Marge Yang <marge.yang@tw.synaptics.com>
> Signed-off-by: Vincent Huang <Vincent.Huang@tw.synaptics.com>
> ---
> drivers/input/rmi4/rmi_f12.c | 41 +++++++++++++++++++++++++++---------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> index 7e97944f7616..b8dfb9ffdbf5 100644
> --- a/drivers/input/rmi4/rmi_f12.c
> +++ b/drivers/input/rmi4/rmi_f12.c
> @@ -24,6 +24,7 @@ enum rmi_f12_object_type {
> };
>
> #define F12_DATA1_BYTES_PER_OBJ 8
> +#define RMI_F12_QUERY_RESOLUTION 29
>
> struct f12_data {
> struct rmi_2d_sensor sensor;
> @@ -73,6 +74,8 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
> int pitch_y = 0;
> int rx_receivers = 0;
> int tx_receivers = 0;
> + u16 query_dpm_addr = 0;
> + int dpm_resolution = 0;
>
> item = rmi_get_register_desc_item(&f12->control_reg_desc, 8);
> if (!item) {
> @@ -122,18 +125,36 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
> offset += 4;
> }
>
> - if (rmi_register_desc_has_subpacket(item, 3)) {
> - rx_receivers = buf[offset];
> - tx_receivers = buf[offset + 1];
> - offset += 2;
> - }
> + /* Use the Query DPM feature when the query register exists for resolution. */
> + item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
> + if (item) {
> + offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> + RMI_F12_QUERY_RESOLUTION);
> + query_dpm_addr = fn->fd.query_base_addr + offset;
> + ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
> + if (ret < 0) {
> + dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
> + return -ENODEV;
> + }
> + dpm_resolution = buf[0];
> +
> + sensor->x_mm = sensor->max_x / dpm_resolution;
> + sensor->y_mm = sensor->max_y / dpm_resolution;
> + } else {
> + if (rmi_register_desc_has_subpacket(item, 3)) {
The item variable is NULL in this branch, as it was overwritten just
before the if statement.
This patch causes a NULL pointer dereference:
[ 1.738650] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics, product: S3706B, fw id: 2869618
[ 1.771232] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018
[ 1.771245] Mem abort info:
[ 1.771248] ESR = 0x0000000096000004
[ 1.771254] EC = 0x25: DABT (current EL), IL = 32 bits
[ 1.771261] SET = 0, FnV = 0
[ 1.771266] EA = 0, S1PTW = 0
[ 1.771271] FSC = 0x04: level 0 translation fault
[ 1.771276] Data abort info:
[ 1.771280] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 1.771285] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 1.771291] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 1.771298] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000986a9000
[ 1.771308] [0000000000000018] pgd=0000000000000000, p4d=0000000000000000
[ 1.771326] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 1.771335] Modules linked in: rmi_i2c(+) rmi_core gpi
[ 1.771358] CPU: 1 UID: 0 PID: 165 Comm: udevd Not tainted 6.11.0-rc6-next-20240902-sdm670-00022-g6ab596a153e1-dirty #10
[ 1.771371] Hardware name: Google Inc. MSM sdm670 S4 PVT v1.0 (DT)
[ 1.771378] pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 1.771389] pc : _find_next_bit+0x18/0x78
[ 1.771411] lr : rmi_register_desc_has_subpacket+0x24/0x40 [rmi_core]
[ 1.771444] sp : ffff800080fdb0b0
[ 1.771448] x29: ffff800080fdb0b0 x28: 0000000000000000 x27: 000000000000000c
[ 1.771463] x26: ffff614ec34aa880 x25: ffff800080fdb0f9 x24: ffff614ec34aa958
[ 1.771477] x23: ffff614ec34adc00 x22: ffff614ed8a02880 x21: ffff614ec34aa9c8
[ 1.771492] x20: ffff614ec34adc18 x19: 0000000000000003 x18: 000000005e77a5e3
[ 1.771506] x17: 000000040044ffff x16: ffffb02de6d83ba8 x15: 0000000000000000
[ 1.771520] x14: ffff614ec1a85640 x13: ffff614ec96dc580 x12: 000000003474591d
[ 1.771534] x11: 0000000068d948e4 x10: ffffb02d74a1c000 x9 : 0000000000000000
[ 1.771548] x8 : ffff614ec96dc500 x7 : 0000000000000001 x6 : 0000000000000001
[ 1.771561] x5 : 0000000000000000 x4 : fffffffffffffff8 x3 : 0000000000000000
[ 1.771574] x2 : 0000000000000003 x1 : 0000000000000100 x0 : 0000000000000018
[ 1.771588] Call trace:
[ 1.771593] _find_next_bit+0x18/0x78
[ 1.771608] rmi_f12_probe+0x728/0x960 [rmi_core]
[ 1.771632] rmi_function_probe+0x8c/0x20c [rmi_core]
[ 1.771654] really_probe+0xbc/0x2c0
[ 1.771671] __driver_probe_device+0x78/0x120
[ 1.771686] driver_probe_device+0x3c/0x154
[ 1.771699] __device_attach_driver+0xb8/0x140
[ 1.771713] bus_for_each_drv+0x88/0xe8
[ 1.771727] __device_attach+0xa0/0x190
[ 1.771735] device_initial_probe+0x14/0x20
[ 1.771744] bus_probe_device+0xb4/0xc0
[ 1.771757] device_add+0x578/0x750
[ 1.771769] rmi_register_function+0x64/0xc8 [rmi_core]
[ 1.771792] rmi_create_function+0x11c/0x1c4 [rmi_core]
[ 1.771815] rmi_scan_pdt+0x90/0x1e4 [rmi_core]
[ 1.771837] rmi_init_functions+0x6c/0x13c [rmi_core]
[ 1.771860] rmi_driver_probe+0x130/0x3a0 [rmi_core]
[ 1.771882] really_probe+0xbc/0x2c0
[ 1.771896] __driver_probe_device+0x78/0x120
[ 1.771911] driver_probe_device+0x3c/0x154
[ 1.771925] __device_attach_driver+0xb8/0x140
[ 1.771939] bus_for_each_drv+0x88/0xe8
[ 1.771952] __device_attach+0xa0/0x190
[ 1.771960] device_initial_probe+0x14/0x20
[ 1.771970] bus_probe_device+0xb4/0xc0
[ 1.771983] device_add+0x578/0x750
[ 1.771994] rmi_register_transport_device+0x8c/0x138 [rmi_core]
[ 1.772017] rmi_i2c_probe+0x1b0/0x304 [rmi_i2c]
[ 1.772040] i2c_device_probe+0x130/0x290
[ 1.772056] really_probe+0xbc/0x2c0
[ 1.772070] __driver_probe_device+0x78/0x120
[ 1.772084] driver_probe_device+0x3c/0x154
[ 1.772098] __driver_attach+0x90/0x1a0
[ 1.772111] bus_for_each_dev+0x7c/0xe0
[ 1.772124] driver_attach+0x24/0x30
[ 1.772137] bus_add_driver+0xe4/0x208
[ 1.772150] driver_register+0x68/0x124
[ 1.772159] i2c_register_driver+0x48/0xcc
[ 1.772173] rmi_i2c_driver_init+0x20/0x1000 [rmi_i2c]
[ 1.772185] do_one_initcall+0x60/0x1d4
[ 1.772198] do_init_module+0x5c/0x21c
[ 1.772211] load_module+0x18cc/0x1e60
[ 1.772222] init_module_from_file+0x88/0xd0
[ 1.772234] __arm64_sys_finit_module+0x1c0/0x320
[ 1.772246] invoke_syscall+0x48/0x104
[ 1.772261] el0_svc_common.constprop.0+0x40/0xe0
[ 1.772274] do_el0_svc+0x1c/0x28
[ 1.772287] el0_svc+0x34/0xe0
[ 1.772298] el0t_64_sync_handler+0x120/0x12c
[ 1.772309] el0t_64_sync+0x190/0x194
[ 1.772324] Code: d346fc43 92800004 9ac22084 d37df065 (f8656802)
[ 1.772331] ---[ end trace 0000000000000000 ]---
^ permalink raw reply
* [PATCH v2 3/3] iio: hid-sensor: Use aligned data type for timestamp
From: Andy Shevchenko @ 2024-09-03 17:59 UTC (permalink / raw)
To: Uwe Kleine-König, Andy Shevchenko, Jonathan Cameron,
Srinivas Pandruvada, Basavaraj Natikar, linux-input, linux-iio,
linux-kernel
Cc: Jiri Kosina, Jonathan Cameron, Lars-Peter Clausen,
Lorenzo Bianconi
In-Reply-To: <20240903180218.3640501-1-andriy.shevchenko@linux.intel.com>
Use aligned_s64 for the timestamp field.
Note, the actual data is signed, hence with this we also amend that.
While at it, drop redundant __alignment directive.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/iio/accel/hid-sensor-accel-3d.c | 2 +-
drivers/iio/gyro/hid-sensor-gyro-3d.c | 2 +-
drivers/iio/humidity/hid-sensor-humidity.c | 2 +-
drivers/iio/light/hid-sensor-als.c | 2 +-
drivers/iio/orientation/hid-sensor-incl-3d.c | 2 +-
drivers/iio/orientation/hid-sensor-rotation.c | 2 +-
drivers/iio/position/hid-sensor-custom-intel-hinge.c | 2 +-
drivers/iio/pressure/hid-sensor-press.c | 2 +-
drivers/iio/temperature/hid-sensor-temperature.c | 2 +-
9 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
index 9b7a73a4c48a..431a12171504 100644
--- a/drivers/iio/accel/hid-sensor-accel-3d.c
+++ b/drivers/iio/accel/hid-sensor-accel-3d.c
@@ -28,7 +28,7 @@ struct accel_3d_state {
/* Ensure timestamp is naturally aligned */
struct {
u32 accel_val[3];
- s64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c b/drivers/iio/gyro/hid-sensor-gyro-3d.c
index 59a38bf9459b..d6562bd425f2 100644
--- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
+++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
@@ -27,7 +27,7 @@ struct gyro_3d_state {
struct hid_sensor_hub_attribute_info gyro[GYRO_3D_CHANNEL_MAX];
struct {
u32 gyro_val[GYRO_3D_CHANNEL_MAX];
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
diff --git a/drivers/iio/humidity/hid-sensor-humidity.c b/drivers/iio/humidity/hid-sensor-humidity.c
index bf6d2636a85e..eb1c022f73c8 100644
--- a/drivers/iio/humidity/hid-sensor-humidity.c
+++ b/drivers/iio/humidity/hid-sensor-humidity.c
@@ -18,7 +18,7 @@ struct hid_humidity_state {
struct hid_sensor_hub_attribute_info humidity_attr;
struct {
s32 humidity_data;
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
index 260281194f61..0c1d97aecd71 100644
--- a/drivers/iio/light/hid-sensor-als.c
+++ b/drivers/iio/light/hid-sensor-als.c
@@ -31,7 +31,7 @@ struct als_state {
struct iio_chan_spec channels[CHANNEL_SCAN_INDEX_MAX + 1];
struct {
u32 illum[CHANNEL_SCAN_INDEX_MAX];
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
diff --git a/drivers/iio/orientation/hid-sensor-incl-3d.c b/drivers/iio/orientation/hid-sensor-incl-3d.c
index 8943d5c78bc0..f5e5fb68caf8 100644
--- a/drivers/iio/orientation/hid-sensor-incl-3d.c
+++ b/drivers/iio/orientation/hid-sensor-incl-3d.c
@@ -29,7 +29,7 @@ struct incl_3d_state {
struct hid_sensor_hub_attribute_info incl[INCLI_3D_CHANNEL_MAX];
struct {
u32 incl_val[INCLI_3D_CHANNEL_MAX];
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
diff --git a/drivers/iio/orientation/hid-sensor-rotation.c b/drivers/iio/orientation/hid-sensor-rotation.c
index 5e8cadd5177a..501c312ce752 100644
--- a/drivers/iio/orientation/hid-sensor-rotation.c
+++ b/drivers/iio/orientation/hid-sensor-rotation.c
@@ -20,7 +20,7 @@ struct dev_rot_state {
struct hid_sensor_hub_attribute_info quaternion;
struct {
s32 sampled_vals[4] __aligned(16);
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
diff --git a/drivers/iio/position/hid-sensor-custom-intel-hinge.c b/drivers/iio/position/hid-sensor-custom-intel-hinge.c
index 76e173850a35..6239e2f72a05 100644
--- a/drivers/iio/position/hid-sensor-custom-intel-hinge.c
+++ b/drivers/iio/position/hid-sensor-custom-intel-hinge.c
@@ -39,7 +39,7 @@ struct hinge_state {
const char *labels[CHANNEL_SCAN_INDEX_MAX];
struct {
u32 hinge_val[3];
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
diff --git a/drivers/iio/pressure/hid-sensor-press.c b/drivers/iio/pressure/hid-sensor-press.c
index 956045e2db29..0419bb3c3494 100644
--- a/drivers/iio/pressure/hid-sensor-press.c
+++ b/drivers/iio/pressure/hid-sensor-press.c
@@ -24,7 +24,7 @@ struct press_state {
struct hid_sensor_hub_attribute_info press_attr;
struct {
u32 press_data;
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
diff --git a/drivers/iio/temperature/hid-sensor-temperature.c b/drivers/iio/temperature/hid-sensor-temperature.c
index 0143fd478933..d2209cd5b98c 100644
--- a/drivers/iio/temperature/hid-sensor-temperature.c
+++ b/drivers/iio/temperature/hid-sensor-temperature.c
@@ -18,7 +18,7 @@ struct temperature_state {
struct hid_sensor_hub_attribute_info temperature_attr;
struct {
s32 temperature_data;
- u64 timestamp __aligned(8);
+ aligned_s64 timestamp;
} scan;
int scale_pre_decml;
int scale_post_decml;
--
2.43.0.rc1.1336.g36b5255a03ac
^ permalink raw reply related
* [PATCH v2 2/3] iio: imu: st_lsm6dsx: Use aligned data type for timestamp
From: Andy Shevchenko @ 2024-09-03 17:59 UTC (permalink / raw)
To: Uwe Kleine-König, Andy Shevchenko, Jonathan Cameron,
Srinivas Pandruvada, Basavaraj Natikar, linux-input, linux-iio,
linux-kernel
Cc: Jiri Kosina, Jonathan Cameron, Lars-Peter Clausen,
Lorenzo Bianconi
In-Reply-To: <20240903180218.3640501-1-andriy.shevchenko@linux.intel.com>
Use __aligned_s64 for the timestamp field.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index a3b93566533b..d5e2771042bd 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -444,10 +444,9 @@ struct st_lsm6dsx_hw {
const struct st_lsm6dsx_settings *settings;
struct iio_mount_matrix orientation;
- /* Ensure natural alignment of buffer elements */
struct {
__le16 channels[3];
- s64 ts __aligned(8);
+ aligned_s64 ts;
} scan[ST_LSM6DSX_ID_MAX];
};
--
2.43.0.rc1.1336.g36b5255a03ac
^ permalink raw reply related
* [PATCH v2 0/3] iio: Introduce and use aligned_s64 type
From: Andy Shevchenko @ 2024-09-03 17:59 UTC (permalink / raw)
To: Uwe Kleine-König, Andy Shevchenko, Jonathan Cameron,
Srinivas Pandruvada, Basavaraj Natikar, linux-input, linux-iio,
linux-kernel
Cc: Jiri Kosina, Jonathan Cameron, Lars-Peter Clausen,
Lorenzo Bianconi
Instead of having open coded idea of aligned member, use
a newly defined type like it's done in, e.g., u64 case.
Update a few IIO drivers to show how to use it.
v2 (took only one year from v1, not bad!):
- avoided touching unrelated comments, code lines, etc. (Jonathan)
- used kernel internal type for the in-kernel code (Jonathan)
Andy Shevchenko (3):
types: Complement the aligned types with signed 64-bit one
iio: imu: st_lsm6dsx: Use aligned data type for timestamp
iio: hid-sensor: Use aligned data type for timestamp
drivers/iio/accel/hid-sensor-accel-3d.c | 2 +-
drivers/iio/gyro/hid-sensor-gyro-3d.c | 2 +-
drivers/iio/humidity/hid-sensor-humidity.c | 2 +-
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 3 +--
drivers/iio/light/hid-sensor-als.c | 2 +-
drivers/iio/orientation/hid-sensor-incl-3d.c | 2 +-
drivers/iio/orientation/hid-sensor-rotation.c | 2 +-
drivers/iio/position/hid-sensor-custom-intel-hinge.c | 2 +-
drivers/iio/pressure/hid-sensor-press.c | 2 +-
drivers/iio/temperature/hid-sensor-temperature.c | 2 +-
include/linux/types.h | 3 ++-
include/uapi/linux/types.h | 1 +
12 files changed, 13 insertions(+), 12 deletions(-)
--
2.43.0.rc1.1336.g36b5255a03ac
^ permalink raw reply
* [PATCH v2 1/3] types: Complement the aligned types with signed 64-bit one
From: Andy Shevchenko @ 2024-09-03 17:59 UTC (permalink / raw)
To: Uwe Kleine-König, Andy Shevchenko, Jonathan Cameron,
Srinivas Pandruvada, Basavaraj Natikar, linux-input, linux-iio,
linux-kernel
Cc: Jiri Kosina, Jonathan Cameron, Lars-Peter Clausen,
Lorenzo Bianconi
In-Reply-To: <20240903180218.3640501-1-andriy.shevchenko@linux.intel.com>
Some user may want to use aligned signed 64-bit type.
Provide it for them.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
include/linux/types.h | 3 ++-
include/uapi/linux/types.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/types.h b/include/linux/types.h
index 2bc8766ba20c..2d7b9ae8714c 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -115,8 +115,9 @@ typedef u64 u_int64_t;
typedef s64 int64_t;
#endif
-/* this is a special 64bit data type that is 8-byte aligned */
+/* These are the special 64-bit data types that are 8-byte aligned */
#define aligned_u64 __aligned_u64
+#define aligned_s64 __aligned_s64
#define aligned_be64 __aligned_be64
#define aligned_le64 __aligned_le64
diff --git a/include/uapi/linux/types.h b/include/uapi/linux/types.h
index 6375a0684052..48b933938877 100644
--- a/include/uapi/linux/types.h
+++ b/include/uapi/linux/types.h
@@ -53,6 +53,7 @@ typedef __u32 __bitwise __wsum;
* No conversions are necessary between 32-bit user-space and a 64-bit kernel.
*/
#define __aligned_u64 __u64 __attribute__((aligned(8)))
+#define __aligned_s64 __s64 __attribute__((aligned(8)))
#define __aligned_be64 __be64 __attribute__((aligned(8)))
#define __aligned_le64 __le64 __attribute__((aligned(8)))
--
2.43.0.rc1.1336.g36b5255a03ac
^ permalink raw reply related
* Re: [PATCH v1 00/22] iio: use dev_get_platdata() to access platform_data
From: Andy Shevchenko @ 2024-09-03 17:57 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Michael Hennerich,
Antoniu Miclaus, Jinjie Ruan, Lorenzo Bianconi,
Srinivas Pandruvada, Basavaraj Natikar, linux-input, linux-iio,
linux-kernel
Cc: Jiri Kosina, Jonathan Cameron, Lars-Peter Clausen
In-Reply-To: <20240902222824.1145571-1-andy.shevchenko@gmail.com>
On Tue, Sep 03, 2024 at 01:16:45AM +0300, Andy Shevchenko wrote:
> Unify how IIO drivers access platform_data field of struct device.
> In simple and straightforward cases constify the local variables.
>
> (Not tested)
Jonathan, in case you are fine with the series, feel free to squash, e.g.,
changes against hid-sensor drivers.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH HID 6/7] HID: bpf: Allow to control the connect mask of hid-generic from BPF
From: Benjamin Tissoires @ 2024-09-03 16:32 UTC (permalink / raw)
To: Peter Hutterer
Cc: Jiri Kosina, Vicki Pfau, Shuah Khan, Jonathan Corbet, linux-input,
linux-kselftest, linux-kernel, bpf, linux-doc
In-Reply-To: <20240903055745.GB968953@quokka>
On Sep 03 2024, Peter Hutterer wrote:
> On Tue, Sep 03, 2024 at 01:14:36AM +0900, Benjamin Tissoires wrote:
> > We make struct hid_device_id writeable and use the .driver_data field
> > of hid-generic as the connect mask.
>
> I think this needs to be spelled out a bit more: for this to work the
> driver *must* be hid-generic, otherwise this doesn't work. But I'm a bit
> confused why we have a custom fields for force/ignore driver but
> whether the device is connected (and thus uses the driver) is hidden in
> an effectively undocumented private field of one specific driver.
It's hid-generic only because that is the less intrusive approach. I'm
not sure we want an override from BPF for any drivers, as suddenly you
will get some harder-than-required bugs with drivers not exposing input
when they should.
>
> Wouldn't it be easier to add another boolean (or enum entry, see my
> other comment) to hid_bpf_driver? This way *how* it happens is hidden
> from the API as well - you say "hidraw only please" and the kernel does
> the rest (through hid-generic or otherwise).
I thought at that, but again, given that I wanted to enable this only
for hid-generic, it felt weird to have a field that works for just one
driver.
Also, after I sent the series, I realized that it was probably not great
for Steam/SDL: today they basically set uaccess on the hidraw nodes, but
now we are going to require some root permissions to disable the event
node.
I'll need to think more but we probably need something more like
udev-hid-bpf where you can load the "disable event node on hidraw open"
BPF once, and forget about it, and make this bpf recognize that
SDL/Steam is opening the hidraw node, and therefore it needs to
reconnect the device. But this is not possible to do with this series
(and maybe not with the current infrastructure).
Cheers,
Benjamin
>
> Cheers,
> Peter
>
> >
> > This way, we can control from a HID-BPF program if a device needs to
> > be exported through hidraw and/or hid-input mainly.
> >
> > This is useful in case we want to have a third party program that directly
> > talks to the hidraw node and we don't want regular input events to be
> > emitted. This third party program can load a BPF program that instructs
> > hid-generic to rebind on the device with hidraw only and then open the
> > hidraw node itself.
> >
> > When the application is closed, the BPF program is unloaded and the normal
> > driver takes back the control of the device.
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> > drivers/hid/bpf/hid_bpf_struct_ops.c | 1 +
> > drivers/hid/hid-core.c | 14 ++++++++------
> > drivers/hid/hid-generic.c | 5 +++--
> > 3 files changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > index 1e13a22f73a1..bb755edd02f0 100644
> > --- a/drivers/hid/bpf/hid_bpf_struct_ops.c
> > +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > @@ -80,6 +80,7 @@ static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log,
> > WRITE_RANGE(hid_device, name, true),
> > WRITE_RANGE(hid_device, uniq, true),
> > WRITE_RANGE(hid_device, phys, true),
> > + WRITE_RANGE(hid_device_id, driver_data, false),
> > WRITE_RANGE(hid_bpf_driver, force_driver, false),
> > WRITE_RANGE(hid_bpf_driver, ignore_driver, false),
> > };
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 7845f0a789ec..2bd279b23aa4 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -2637,15 +2637,17 @@ EXPORT_SYMBOL_GPL(hid_compare_device_paths);
> >
> > static bool hid_check_device_match(struct hid_device *hdev,
> > struct hid_driver *hdrv,
> > - const struct hid_device_id **id)
> > + struct hid_device_id *id)
> > {
> > + const struct hid_device_id *_id = hid_match_device(hdev, hdrv);
> > int ret;
> >
> > - *id = hid_match_device(hdev, hdrv);
> > - if (!*id)
> > + if (!_id)
> > return false;
> >
> > - ret = call_hid_bpf_driver_probe(hdev, hdrv, *id);
> > + memcpy(id, _id, sizeof(*id));
> > +
> > + ret = call_hid_bpf_driver_probe(hdev, hdrv, id);
> > if (ret)
> > return ret > 0;
> >
> > @@ -2662,7 +2664,7 @@ static bool hid_check_device_match(struct hid_device *hdev,
> >
> > static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
> > {
> > - const struct hid_device_id *id;
> > + struct hid_device_id id;
> > int ret;
> >
> > if (!hid_check_device_match(hdev, hdrv, &id))
> > @@ -2677,7 +2679,7 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
> > hdev->driver = hdrv;
> >
> > if (hdrv->probe) {
> > - ret = hdrv->probe(hdev, id);
> > + ret = hdrv->probe(hdev, &id);
> > } else { /* default probe */
> > ret = hid_open_report(hdev);
> > if (!ret)
> > diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
> > index f9db991d3c5a..5cd1f3a79a4b 100644
> > --- a/drivers/hid/hid-generic.c
> > +++ b/drivers/hid/hid-generic.c
> > @@ -64,11 +64,12 @@ static int hid_generic_probe(struct hid_device *hdev,
> > if (ret)
> > return ret;
> >
> > - return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > + return hid_hw_start(hdev, id->driver_data);
> > }
> >
> > static const struct hid_device_id hid_table[] = {
> > - { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) },
> > + { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID),
> > + .driver_data = HID_CONNECT_DEFAULT },
> > { }
> > };
> > MODULE_DEVICE_TABLE(hid, hid_table);
> >
> > --
> > 2.46.0
> >
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: remove unneeded file entry in INPUT section
From: Dmitry Torokhov @ 2024-09-03 15:47 UTC (permalink / raw)
To: Lukas Bulwahn; +Cc: linux-input, kernel-janitors, linux-kernel, Lukas Bulwahn
In-Reply-To: <20240903093948.122957-1-lukas.bulwahn@redhat.com>
On Tue, Sep 03, 2024 at 11:39:48AM +0200, Lukas Bulwahn wrote:
> From: Lukas Bulwahn <lukas.bulwahn@redhat.com>
>
> Commit b9401c658d2c ("MAINTAINERS: add gameport.h, serio.h and uinput.h to
> INPUT section") adds further header files in ./include/linux/ and
> ./include/uapi/linux to the INPUT section, but the file
> ./include/linux/uinput.h does not exist since commit a11bc476b987 ("Input:
> uinput - fold header into the driver proper") removed this header file
> in 2017.
>
> Fortunately, ./scripts/get_maintainer.pl --self-test=patterns complains
> about a broken reference. Remove the file entry referring to the
> non-existing header file in the INPUT section.
TIL about get_maintainer.pl --self-test.
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH HID 4/7] HID: bpf: allow BPF programs to force using hid-generic
From: Benjamin Tissoires @ 2024-09-03 15:05 UTC (permalink / raw)
To: Peter Hutterer
Cc: Jiri Kosina, Vicki Pfau, Shuah Khan, Jonathan Corbet, linux-input,
linux-kselftest, linux-kernel, bpf, linux-doc
In-Reply-To: <20240903053656.GA968953@quokka>
On Sep 03 2024, Peter Hutterer wrote:
> On Tue, Sep 03, 2024 at 01:14:34AM +0900, Benjamin Tissoires wrote:
> > The use case is when we fix a device through HID-BPF, 99% of the cases
> > we want the device to use hid-generic now instead of a dedicated device.
>
> s/dedicated device/dedicated driver/ in the commit message
>
> > That's because the dedicated device might also want to change the report
> > descriptor, or will be handling the device in a different way the new
> > fixed device is using.
> >
> > In hid-core, after matching for the device (so that we only call this new
> > hook on compatible drivers), we call for `.hid_bpf_driver_probe`.
> > The function can not communicate with the device because it is not yet
> > started, but it can make educated guesses and decide to:
> > - let hid-core decide by itself
> > - force the use of this driver (by comparing the provided name with
> > "hid-generic" for instance)
> > - force hid-core to ignore this driver for this device.
> >
> > For API stability, we don't rely on a bitfield or a return value for
> > chosing hid-core behavior. We simply have a couple of writeable fields
> > in the new struct hid_bpf_driver, and then hid-core can make its educated
> > decision.
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> > Documentation/hid/hid-bpf.rst | 2 +-
> > drivers/hid/bpf/hid_bpf_dispatch.c | 31 ++++++++++++++++++++++++++++
> > drivers/hid/bpf/hid_bpf_struct_ops.c | 3 +++
> > drivers/hid/hid-core.c | 6 ++++++
> > include/linux/hid_bpf.h | 40 ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/hid/hid-bpf.rst b/Documentation/hid/hid-bpf.rst
> > index 5939eeafb361..05a43f11cdab 100644
> > --- a/Documentation/hid/hid-bpf.rst
> > +++ b/Documentation/hid/hid-bpf.rst
> > @@ -190,7 +190,7 @@ User API data structures available in programs:
> > -----------------------------------------------
> >
> > .. kernel-doc:: include/linux/hid_bpf.h
> > - :identifiers: hid_bpf_ctx
> > + :identifiers: hid_bpf_ctx hid_bpf_driver
> >
> > Available API that can be used in all HID-BPF struct_ops programs:
> > ------------------------------------------------------------------
> > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> > index a272a086c950..2df136d64152 100644
> > --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> > @@ -189,6 +189,37 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *s
> > }
> > EXPORT_SYMBOL_GPL(call_hid_bpf_rdesc_fixup);
> >
> > +int call_hid_bpf_driver_probe(struct hid_device *hdev, struct hid_driver *hdrv,
> > + const struct hid_device_id *id)
> > +{
> > + struct hid_bpf_driver drv = { 0 };
> > + struct hid_bpf_ops *e;
> > + int idx;
> +
> > + if (strscpy(drv.name, hdrv->name, sizeof(drv.name)) < 0)
> > + return 0;
> > +
> > + idx = srcu_read_lock(&hdev->bpf.srcu);
> > + list_for_each_entry_srcu(e, &hdev->bpf.prog_list, list,
> > + srcu_read_lock_held(&hdev->bpf.srcu)) {
> > + if (!e->hid_driver_probe)
> > + continue;
> > +
> > + e->hid_driver_probe(hdev, &drv, id);
> > + }
> > +
> > + srcu_read_unlock(&hdev->bpf.srcu, idx);
> > +
> > + if (drv.force_driver)
> > + return 1;
> > +
> > + if (drv.ignore_driver)
> > + return -1;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(call_hid_bpf_driver_probe);
> > +
> > static int device_match_id(struct device *dev, const void *id)
> > {
> > struct hid_device *hdev = to_hid_device(dev);
> > diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > index cd696c59ba0f..1e13a22f73a1 100644
> > --- a/drivers/hid/bpf/hid_bpf_struct_ops.c
> > +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > @@ -46,6 +46,7 @@ static int hid_bpf_ops_check_member(const struct btf_type *t,
> > case offsetof(struct hid_bpf_ops, hid_rdesc_fixup):
> > case offsetof(struct hid_bpf_ops, hid_hw_request):
> > case offsetof(struct hid_bpf_ops, hid_hw_output_report):
> > + case offsetof(struct hid_bpf_ops, hid_driver_probe):
> > break;
> > default:
> > if (prog->sleepable)
> > @@ -79,6 +80,8 @@ static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log,
> > WRITE_RANGE(hid_device, name, true),
> > WRITE_RANGE(hid_device, uniq, true),
> > WRITE_RANGE(hid_device, phys, true),
> > + WRITE_RANGE(hid_bpf_driver, force_driver, false),
> > + WRITE_RANGE(hid_bpf_driver, ignore_driver, false),
> > };
> > #undef WRITE_RANGE
> > const struct btf_type *state = NULL;
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 988d0acbdf04..7845f0a789ec 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -2639,10 +2639,16 @@ static bool hid_check_device_match(struct hid_device *hdev,
> > struct hid_driver *hdrv,
> > const struct hid_device_id **id)
> > {
> > + int ret;
> > +
> > *id = hid_match_device(hdev, hdrv);
> > if (!*id)
> > return false;
> >
> > + ret = call_hid_bpf_driver_probe(hdev, hdrv, *id);
> > + if (ret)
> > + return ret > 0;
> > +
> > if (hdrv->match)
> > return hdrv->match(hdev, hid_ignore_special_drivers);
> >
> > diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
> > index d4d063cf63b5..20693c218857 100644
> > --- a/include/linux/hid_bpf.h
> > +++ b/include/linux/hid_bpf.h
> > @@ -9,6 +9,7 @@
> > #include <uapi/linux/hid.h>
> >
> > struct hid_device;
> > +struct hid_driver;
> >
> > /*
> > * The following is the user facing HID BPF API.
> > @@ -80,6 +81,22 @@ struct hid_ops {
> >
> > extern struct hid_ops *hid_ops;
> >
> > +/**
> > + * struct hid_bpf_driver - User accessible data for the ``hid_bpf_probe``
> > + * struct_ops
> > + *
> > + * @name: the name of the driver currently being treated
> > + * @force_driver: set this to ``true`` to force hid-core to use this driver,
> > + * bypassing any further decision made by this driver
> > + * @ignore_driver: set this to ``true`` to force hid-core to ignore this driver,
> > + * bypassing any further decision made by this driver
>
> If I set both to false or true, what happens? The two seem to be
force_driver has priority over ignore_driver.
> mutually exclusive, in userspace I'd use an enum here to have a
> NOOP/FORCE_DRIVER/IGNORE_DRIVER value range (that can be extended later).
> Maybe something like that is an option?
enum also has the advantage to be exported in vmlinux.h.
FWIW, the idea behind adding new fields in a struct was to get the
backward compatibility for free. Because the verifier/relocator will see
if we are using the correct field entries.
OTOH, maybe we can make the function return the afformended enum, and
drop those two fields.
I think we should probably abort processing of any bpf sets the return
value to anything else than NOOP.
I'll work a little bit more on that.
Cheers,
Benjamin
>
> > + */
> > +struct hid_bpf_driver {
> > + __u8 name[64];
> > + bool force_driver;
> > + bool ignore_driver;
> > +};
> > +
> > /**
> > * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to attach HID-BPF
> > * programs to a HID device
> > @@ -178,6 +195,25 @@ struct hid_bpf_ops {
> > */
> > int (*hid_hw_output_report)(struct hid_bpf_ctx *ctx, u64 source);
> >
> > + /**
> > + * @hid_driver_probe: called before the kernel ``.probe()`` function
> > + *
> > + * It has the following arguments:
> > + *
> > + * ``hdev``: The HID device kernel representation
> > + *
> > + * ``hdrv``: A BPF partially writeable representation of a HID driver
> > + *
> > + * ``id``: The device match structure found in the driver
> > + *
> > + * Note that the device has not been started yet, and thus kfuncs like
> > + * ``hid_hw_output_report`` will likely fail.
>
> Just to confirm, I can access the device's report descriptor though? For
> the devices that we're looking at (e.g. the foot pedals pretending to be
> an apple keyboard) the driver name and what we can set in HID_BPF_CONFIG
> are not going to be enough, we'll have to check the rdesc too.
>
> Cheers,
> Peter
>
> > + *
> > + * This function is useful to force/ignore a given supported HID driver,
> > + * by writing ``true`` in ``hdrv->force_driver`` or ``hdrv->ignore_driver``
> > + */
> > + void (*hid_driver_probe)(struct hid_device *hdev, struct hid_bpf_driver *hdrv,
> > + const struct hid_device_id *id);
> >
> > /* private: do not show up in the docs */
> > struct hid_device *hdev;
> > @@ -213,6 +249,8 @@ void hid_bpf_disconnect_device(struct hid_device *hdev);
> > void hid_bpf_destroy_device(struct hid_device *hid);
> > int hid_bpf_device_init(struct hid_device *hid);
> > u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size);
> > +int call_hid_bpf_driver_probe(struct hid_device *hdev, struct hid_driver *hdrv,
> > + const struct hid_device_id *id);
> > #else /* CONFIG_HID_BPF */
> > static inline u8 *dispatch_hid_bpf_device_event(struct hid_device *hid, enum hid_report_type type,
> > u8 *data, u32 *size, int interrupt,
> > @@ -228,6 +266,8 @@ static inline int hid_bpf_connect_device(struct hid_device *hdev) { return 0; }
> > static inline void hid_bpf_disconnect_device(struct hid_device *hdev) {}
> > static inline void hid_bpf_destroy_device(struct hid_device *hid) {}
> > static inline int hid_bpf_device_init(struct hid_device *hid) { return 0; }
> > +static inline int call_hid_bpf_driver_probe(struct hid_device *hdev, struct hid_driver *hdrv,
> > + const struct hid_device_id *id) { return 0; }
> > /*
> > * This specialized allocator has to be a macro for its allocations to be
> > * accounted separately (to have a separate alloc_tag). The typecast is
> >
> > --
> > 2.46.0
> >
^ permalink raw reply
* Re: [PATCH] HID: hid-sensor-custom: Convert comma to semicolon
From: Jiri Kosina @ 2024-09-03 12:00 UTC (permalink / raw)
To: Chen Ni
Cc: jic23, srinivas.pandruvada, bentiss, linux-input, linux-iio,
linux-kernel
In-Reply-To: <20240903025010.493843-1-nichen@iscas.ac.cn>
On Tue, 3 Sep 2024, Chen Ni wrote:
> Replace a comma between expression statements by a semicolon.
Applied, thank you.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH RESEND] hid: add patch for sis multitouch format
From: Jiri Kosina @ 2024-09-03 11:58 UTC (permalink / raw)
To: tammy tseng; +Cc: bentiss, linux-input, linux-kernel, tammy_tseng
In-Reply-To: <20240806082531.1353207-2-tammy0524@gmail.com>
On Tue, 6 Aug 2024, tammy tseng wrote:
> The patch is to add proper quirks for sis multitouch format
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox